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

Bloop doesn't work with RootModule #2486

Closed
lolgab opened this issue Apr 30, 2023 · 21 comments
Closed

Bloop doesn't work with RootModule #2486

lolgab opened this issue Apr 30, 2023 · 21 comments
Milestone

Comments

@lolgab
Copy link
Member

lolgab commented Apr 30, 2023

Given a build.sc like

import $ivy.`com.lihaoyi::mill-contrib-bloop:0.11.0-M8`

import mill._
import mill.scalalib._

object root extends RootModule with ScalaModule {
  def scalaVersion = "3.2.2"
}

Building the Bloop configuration with mill mill.contrib.bloop.Bloop/install returns:

> mill mill.contrib.bloop.Bloop/install
[29/32] root.bloop.config 
1 targets failed
root.bloop.config os.PathError$InvalidSegment: [] is not a valid path segment. OS-Lib does not allow empty path segments If you are dealing with path-strings coming from external sources, use the Path(...) or RelPath(...) constructor calls to convert them. 
    os.BasePath$.fail$1(Path.scala:125)
    os.BasePath$.checkSegment(Path.scala:142)
    os.PathChunk$StringPathChunk.<init>(Path.scala:25)
    os.PathChunk$.StringPathChunk(Path.scala:24)
    mill.contrib.bloop.BloopImpl.out$1(BloopImpl.scala:158)
    mill.contrib.bloop.BloopImpl.$anonfun$bloopConfig$58(BloopImpl.scala:409)
@lihaoyi
Copy link
Member

lihaoyi commented May 6, 2023

Currently, mill-contrib-bloop uses the name of the module foo.bar.baz as the file name on disk. That doesn't work when the name of the module is empty, which is the case for the RootModule

I'm not super familiar with what mill-contrib-bloop does, but it seems the solution is to change the naming convention to either prefix it module.foo.bar.baz, or make it folder-based e.g. foo/bar/baz/module.json. Either case would make it robust enough to work for the root module

@lefou
Copy link
Member

lefou commented May 6, 2023

Folder based named will not work, as we allow multiple modules to have the same millSourcePath. I think we need to come up with a unique module identifier for root modules in general, as I've hit this issues with the empty name in other places, especially when formatting output, too.

@lihaoyi
Copy link
Member

lihaoyi commented May 6, 2023

Folder based names can be based on the module's path in the out/' folder, which is guaranteed to he unique. No need to bring millSourcePath` into it

That's not to say that the empty toString for the root module isn't a nuisance, just that fixing it can come separately from this specific issue

@lefou
Copy link
Member

lefou commented May 6, 2023

RootModules can't be completely free ordered, they follow one stringent layout. We already map the top-level module to mill-build, so we could also map each parent of mill-build to a hard-coded name, like mill-build.mill-build or mill-build.parent.

@lefou
Copy link
Member

lefou commented May 6, 2023

We should aim for a solution, consistent with BSP (where this problem should also arise) and cli usage/reporting.

@lolgab
Copy link
Member Author

lolgab commented May 6, 2023

A possible solution that comes to my mind is to use the RootModule object name as identifier and then scope everything else as root. if you are using a RootModule, otherwise we use the current naming.

object root extends Root Module {
  object foo extends Module
}

In this case the identifiers are root and root.foo

While in case you don't use a RootModule you have:

object foo extends Module

And the identifier is foo as it is nowadays.

This also maintains some sort of backwards compatibility for people not using RootModule

@lefou
Copy link
Member

lefou commented May 6, 2023

Hm, I've not though very long about it, but I think using RootModule should be almost transparent to the rest of the module hierarchy. The idea of @lolgab will introduce a complete break of all module names once you introduce or implement a RootModule yourself. Also, IIUC, @lihaoyi 's motivations of using a RootModule was to provide shorter target identifiers, so that a simple mill compile can be used, instead of mill root.compile.

@lihaoyi
Copy link
Member

lihaoyi commented May 6, 2023

what @lolgab suggests is possible, but does add some divergence between how things are references from the CLI (without the root. prefix) and how things are printed/referenced elsewhere (with the prefix).

How about just making the root module name root-module? We already used dashed names elsewhere where we want to avoid collisions (e.g. mill-build, mill-profile.json), and this would make it pretty clear textually what the name is referring to

@lefou
Copy link
Member

lefou commented May 6, 2023

Normally, the root module is transparent and only modules defined within it have a name.

In BSP/bloop we also want to map the build.sc itself, so we came up with a sythetic module mill-build.

Now we introduced some special singleton module of type RootModule (which is called root in the OP). The question is, how to map it to a BSP module name. We could just use mill-build.<name of the root module>, which is in this case mill-build.root. This would fit naturally IMHO. It's itself some kind of synthetic module, so we have it under the already sythetic module tree of mill-build.

@lolgab
Copy link
Member Author

lolgab commented May 6, 2023

My idea was to have special naming only in BSP/Bloop but yes, the downside is that there is a divergence between CLI and BSP/Bloop.
If we don't want to touch the other modules, then we need to reserve a name for it, like $root or something like that, use that name for Bloop/BSP only and fail the builds if a user defines a module named $root to avoid collisions.

@lefou
Copy link
Member

lefou commented May 6, 2023

Why isn't the root: RootModule from this example just mapped to root in the first place? For the CLI perspective (where we want to run things), it makes sense to hide it, but in BSP, it's just another module we want to edit. Also, there can't be any collision, as a user can't provide two root module in the same level, as both are defined in the same build.sc and have to match the Scala language rules.

@lihaoyi
Copy link
Member

lihaoyi commented May 6, 2023

I'd be ok with just using the prefixed name for Bloop/BSP.

One consideration is in the implementation. We currently discard the "enclosing" RootModule very early in MillBuildBootstrap, such that the user-defined RootModule takes over for all intents and purposes. That helps keep things consistent between default/user-defined root modules, but makes it trickier to go fish out the name of the user-defined root module to go prefix things with. Definitely doable though

@lihaoyi
Copy link
Member

lihaoyi commented May 6, 2023

Now we introduced some special singleton module of type RootModule (which is called root in the OP). The question is, how to map it to a BSP module name. We could just use mill-build.<name of the root module>, which is in this case mill-build.root. This would fit naturally IMHO. It's itself some kind of synthetic module, so we have it under the already sythetic module tree of mill-build.

mill-build is probably the wrong name here, since it kind of refers to the meta-build, and the default root module is called build (after the file name build.sc). So build.blah would be the natural naming, if we wanted to a "fully qualified" name including the root module

@lolgab
Copy link
Member Author

lolgab commented May 6, 2023

If you don't want to scope other modules as I suggested in the beginning there can be collision.

object root extends RootModule {
  object root extends Module
}

How would you identify these two modules?

@lihaoyi
Copy link
Member

lihaoyi commented May 6, 2023

My idea was to have special naming only in BSP/Bloop but yes, the downside is that there is a divergence between CLI and BSP/Bloop.

After thinking about this a bit, I think it's probably fine if there's some divergence. If the BSP/Bloop names are just used for generated files on disk, it doesn't seem like something that a user needs to care about, and seems unlikely to cause confusion

@lefou
Copy link
Member

lefou commented May 6, 2023

The mill-build is effectively the synthetic BSP representation of build (defined in build.sc). That name is more in line with what other tools do in BSP and makes it easier to attribute it to Mill, then just using the name build. In that regard, mill-build.root would be quite correct.

@lihaoyi
Copy link
Member

lihaoyi commented May 6, 2023

I'd be worried about confusion with the mill-build/ folder we use for meta-builds. For example, one idea for making the meta-build directly runnable would be to reference it similar to an external module, via

./mill mill-build/foo

But if the root build.sc stuff is put under mill-build.foo, I can imagine it causing confusion.

Some ideas:

  1. What if we called it mill-root-module or mill-root instead of mill-build? That would namespace it under Mill, give it a nice descriptive name, and avoid name confusion with the meta-build

  2. This may be a bit invasive, but what if we renamed build.sc to mill-build.sc? That would make the mill-build.foo naming convention line up perfectly, and would also make the mill-build.sc script and mill-build/ folder line up nicely, making the relationship between them much clearer than it is now, while also namespacing the files under mill- to make their relationship to the Mill build tool much clearer than the build.sc naming today.

@lefou
Copy link
Member

lefou commented May 6, 2023

1. What if we called it `mill-root-module` or `mill-root` instead of `mill-build`? That would namespace it under Mill, give it a nice descriptive name, and avoid name confusion with the meta-build

This sound attractive, but has the issue that this only feels intuitive, as long as the RootModule is also called root. Maybe, we want the name root not only being a convention but a requirement?

2. This may be a bit invasive, but what if we renamed `build.sc` to `mill-build.sc`? That would make the `mill-build.foo` naming convention line up perfectly, and would also make the `mill-build.sc` script and `mill-build/` folder line up nicely, making the relationship between them much clearer than it is now, while also namespacing the files under `mill-` to make their relationship to the Mill build tool much clearer than the `build.sc` naming today.

I think we are a bit late with this change. Many accepted the build.sc name and the longer name doesn't really solve the issue, it just aligns the names. Also, I'd really love to implement a -f option to give Mill an alternative build file instead of a hard-coded one. Many other tools support such option, and it makes testing derivations of a build easier. This is another story, but on this background, the name change is even less attractive.

It turns out this discussion is more about a general mapping of the various files, modules and targets, and not so much an isolated BSP/bloop topic anymore. And once, we tackle this topic, we might also think how we want to handle foreign modules, which currently show up in BSP and in progress, but can't be called from the cli, which is really unfortunate.

@lolgab
Copy link
Member Author

lolgab commented May 6, 2023

Maybe, we want the name root not only being a convention but a requirement?

I like the idea. root is the most appropriate name for a RootModule.

@lihaoyi
Copy link
Member

lihaoyi commented May 6, 2023

standardizing the name sounds reasonable, but should we call it root or build? The normal non-user-defined root module is called build, and i do see the name used in build files e.g. build.foo

@lihaoyi
Copy link
Member

lihaoyi commented May 13, 2023

Fixed by #2415 which gives it the name root-module

@lihaoyi lihaoyi closed this as completed May 13, 2023
@lefou lefou added this to the 0.11.0-M9 milestone May 13, 2023
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

No branches or pull requests

3 participants