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

Foreign modules in T.workspace are not stored in out/foreign-modules/ #2130

Closed
lolgab opened this issue Nov 19, 2022 · 5 comments · Fixed by #2132
Closed

Foreign modules in T.workspace are not stored in out/foreign-modules/ #2130

lolgab opened this issue Nov 19, 2022 · 5 comments · Fixed by #2132
Milestone

Comments

@lolgab
Copy link
Member

lolgab commented Nov 19, 2022

If you have to .sc files in the root, the other files modules are not stored in out/foreign-modules, colliding when the modules have the same name.
Example:

// build.sc
import mill._
import mill.scalalib._
import $file.other

object foo extends JavaModule {
  def moduleDeps = Seq(other.foo)
}
// other.sc
import mill._
import mill.scalalib._
import $file.other

object foo extends JavaModule {
  // EDIT: the following line needs to be commented out
  def moduleDeps = Seq(other.foo)
}
@lefou
Copy link
Member

lefou commented Nov 19, 2022

You find me puzzled. Since when is it possible to declare Mill modules in objects in non-build.sc files? Maybe, I'm remembering it wrong, but I'm pretty sure it didn't work some major versions ago. How do you invoke these targets?

So, after the first shock, back to the point. I think you posted the wrong content for other.sc, as it is including and referring to itself creating an infinite loop and resulting in a StackOverflowError.

The right solution depends somewhat on whether we stick with Ammonite or go an alternative route, which is based on a new MillBuildModule (see #1972). In the latter case, having two object foo in the same scope might be a compile error and we don't have to deal with it in Mills task naming and caching.

If we want to stick with Ammonite (or just want a fix as soon as possible), we need to encode the name of the include file name. We currently only do this, if its location is different from T.workspace:

if (path != wd) {

@lolgab
Copy link
Member Author

lolgab commented Nov 19, 2022

How do you invoke these targets?

I don't. I use other files to put modules that contain generated code or something I don't have to interact with, so the build.sc is cleaner, takes less time to compile, and also I can take advantage of the precise build cache invalidation I implemented in Mill 0.10.1.

The right solution depends somewhat on whether we stick with Ammonite or go an alternative route, which is based on a new MillBuildModule (see #1972). In the latter case, having two object foo in the same scope might be a compile error and we don't have to deal with it in Mills task naming and caching.

Since the MillBuildModule doesn't even exist yet (and if it exists is not the default nor stable), I think we should fix this in current way to define Mill build, which is Ammonite (a thing I have a love/hate relationship with).

From a first look the solution would to to check the build file path and use foreign-modules if it is different than the main build.sc (wd / "build.sc" ?).

I would love to see in the future a unopinionated approach where users can decide if they want to use Ammonite, scala-cli or a plain Scala build with a meta mill build. The last would be my favourite since it's the least magical, but at the same time I would not like to force any decision to other users that are happy with Ammonite.

@lolgab
Copy link
Member Author

lolgab commented Nov 19, 2022

How did I find the bug?
I was trying to reproduce another weird behavior in a build I have and I forgot to put my second file in the project subdirectory. Then I noticed that no foreign-modules was created. Can be very weird if someone just moves a file around.

@lefou
Copy link
Member

lefou commented Nov 19, 2022

I agree, we should fix it in the current implementation.

@lefou
Copy link
Member

lefou commented Nov 21, 2022

I'm still a bit undecided, whether this is an issue with Mill or Ammonite. It works, as .sc files are always wrapped in a object equally named as the file. So, to be really consistent, we always need to include the object name. I have a draft PR ready, #2132, so you can test, review and discuss the implications. I think, it makes working with foreign modules a bit less convenient.

@lefou lefou closed this as completed Nov 21, 2022
@lefou lefou reopened this Nov 21, 2022
@lefou lefou linked a pull request Nov 21, 2022 that will close this issue
lefou added a commit to lefou/mill that referenced this issue Nov 21, 2022
lefou added a commit that referenced this issue Nov 27, 2022
With this change, we always give included files a unique foreign module
path. This makes the `dest` path more consistent and robust.

Unfortunately, this slightly changes the encoding of foreign modules
names. This is no issue when writing `.sc` files, but it will be visible
in the progress output and in BSP client and IntelliJ Idea. E.g. the
name of an included file in directory `proj1` containing a module
`proj1`, will now have the module name
"foreign-modules.proj1.build.proj1", whereas previously it was
"foreign-modules.proj1.proj1".

* Fix #2130

Pull request: #2132
@lefou lefou added this to the 0.10.10 milestone Nov 27, 2022
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 a pull request may close this issue.

2 participants