Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix sourceItem for mill-build #1349

Merged
merged 1 commit into from
May 29, 2021

Conversation

camper42
Copy link
Contributor

due to hardcode sourceItem(evaluator.rootModule.millSourcePath / "src", generated = false),
Metals not recognize build.sc as source file when user choose mill-bsp build server,
no auto-complete and hover tooltips available

This commit fix #1159

due to hardcode `sourceItem(evaluator.rootModule.millSourcePath / "src", generated = false)`,
Metals not recognize build.sc as source file when user choose mill-bsp build server,
no auto-complete and hover tooltips available

This commit fix com-lihaoyi#1159
Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This hardcodes the build file, which is ok, but will miss any included file.

E.g. in mill we have some additional files:

import $file.ci.shared
import $file.ci.upload

Those will still be missing.

@heksesang
Copy link
Contributor

This hardcodes the build file, which is ok, but will miss any included file.

E.g. in mill we have some additional files:

import $file.ci.shared
import $file.ci.upload

Those will still be missing.

Won't those be imported by regular ammonite support?

@lefou
Copy link
Member

lefou commented May 29, 2021

AFAIK, mill BSP server does not provide any "regular" ammonite support (yet, see #971). So, either you mean the native support of your editor, or some separately started ammonite BSP server. But neither will have any knowledge of the right classpath to be used.

@heksesang
Copy link
Contributor

I was thinking of the editor support. But I forgot about that point, that if the separate files depend on the classpath as well, that support won't be enough.

Does Ammonite have anything that lets us obtain a list of $file-references in a script?

@lefou
Copy link
Member

lefou commented May 29, 2021

I don't know Ammonite well enough, but I think the answer is yet. Ammonite itself seems to provide BSP support, so the best would be to look into that implementation.

Back to this concrete PR, as src is in almost all cases not the right setting, but build.sc is probably too restricted. Have you tried to just set the source path of mill-build root project to . or millSourcePath?

@camper42
Copy link
Contributor Author

Have you tried to just set the source path of mill-build root project to . or millSourcePath?

set source path to . or millSourcePath may be inappropriate too

https://build-server-protocol.github.io/docs/specification.html#build-target-sources-request

  /** Either a text document or a directory. A directory entry must end with a forward
   * slash "/" and a directory entry implies that every nested text document within the
   * directory belongs to this source item.
   */

@heksesang
Copy link
Contributor

heksesang commented May 29, 2021

Have you tried to just set the source path of mill-build root project to . or millSourcePath?

set source path to . or millSourcePath may be inappropriate too

https://build-server-protocol.github.io/docs/specification.html#build-target-sources-request

  /** Either a text document or a directory. A directory entry must end with a forward
   * slash "/" and a directory entry implies that every nested text document within the
   * directory belongs to this source item.
   */

The path will be translated to an absolute path, though? Which would end with a forward slash.

Ah, but every document could be considered to belong to the build target. That is obviously a problem.

@heksesang
Copy link
Contributor

I don't know Ammonite well enough, but I think the answer is yet. Ammonite itself seems to provide BSP support, so the best would be to look into that implementation.

Back to this concrete PR, as src is in almost all cases not the right setting, but build.sc is probably too restricted. Have you tried to just set the source path of mill-build root project to . or millSourcePath?

src would in no normal circumstances match the build files, so if you change it to build.sc, I think it is no less restricted than before in practical terms.

Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@lefou lefou merged commit 1a22595 into com-lihaoyi:main May 29, 2021
@lefou lefou added this to the after 0.9.8 milestone May 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The BSP server returns wrong sources for mill-build target
3 participants