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

Add some helpers to simplify cross-version/cross-platform modules #2406

Merged
merged 8 commits into from Apr 8, 2023

Conversation

lihaoyi
Copy link
Member

@lihaoyi lihaoyi commented Apr 3, 2023

This PR attempts to minimize the boilerplate of defining cross-version cross-platform Scala modules.

Currently, cross-version cross-platform modules involves a lot annoying fiddling with overriding millSourcePath and sources and artifactName and passing around crossScalaVersion and all that. Simple Cross/CrossScalaModules get some of the plumbing handled behind the scenes, but anything more complicated and the plumbing spills out again into plain view. And although Cross/CrossScalaModules have less boilerplate, it's still enough boilerplate you don't want to go around defining them over and over

This PR tries to hide all of them behind helper traits CrossScalaModule.Wrapper and PlatformScalaModule, so your build file only has the logic you care about and none of the plumbing.

Major changes

  1. We replace the artifactName definition, so that instead of reading from millModuleSegments directly, it reads from artifactNameParts: Seq[String]. This makes it easier for us to splice the artifact name parts to remove unwanted segments without needing to do fragile string manipulation

  2. Introduce CrossScalaModule.Wrapper. This can be used when the ScalaModules you are defining are not the direct children of the Cross module, but are nested somewhere inside of it. CrossScalaModule.Wrapper shades the CrossScalaModule trait, so that any CrossScalaModule defined within its body would have the crossScalaVersion automatically wired up and the artifactNameParts properly adjusted to remove the cross segment in the middle of the artifact name

  3. Introduce PlatformScalaModule. This is analogous to CrossScalaModule, in that it adjusts the millSourcePath/artifactNameParts to drop the last segment, and adjusts sources to include the -{jvm,js,native} suffix

Tests

The changes are largely covered by existing tests, including the example/ tests which were adjusted to make use of the new traits

Notes

  1. We cannot make the platform-specific sub-modules into Cross modules like we do with the version-specific sub-modules, since the platform-specific submodules have pretty different types/targets defined on them and Cross modules must all be of the same type

  2. 6-cross-platform-publishing, is a lot better than before, but there is still a significant amount of boilerplate involved, especially the lines around object wrapper. Not sure if there are other things we can do to make it even easier to define these cross-version/platform modules

@lihaoyi lihaoyi marked this pull request as draft April 3, 2023 04:55
@lihaoyi lihaoyi changed the title [WIP] Add some helpers to simplify cross-version/cross-platform modules Add some helpers to simplify cross-version/cross-platform modules Apr 3, 2023
@lihaoyi lihaoyi marked this pull request as ready for review April 3, 2023 05:58
@lihaoyi lihaoyi requested review from lefou and lolgab April 3, 2023 06:14
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.

I find the Wrapper trait name hard to grasp. Maybe we can find some better name for it? And in the example we should definitely use some example domain name instead of wrapper, to make sure, the names are not bound. (They are just coincidentally named "wrapper".)

Otherwise, @lolgab is using multi-platfrom-cross-scala project more frequently than me and also has put some thought and work (a dedictated plugin for that purpose) into this topic, so I'd value his opinion here!

@lefou lefou linked an issue Apr 3, 2023 that may be closed by this pull request
@lolgab
Copy link
Member

lolgab commented Apr 3, 2023

Haven't looked at your solution yet but, if you haven't yet, I suggest you to give a look at my plugin's code: https://github.com/lolgab/mill-crossplatform
One thing that I did was to put the Cross module in the wrapper module instead at the level of the individual platforms modules. To disable some versions from some platforms (when Scala Native didn't support Scala 3 for example) I redefined millModuleDirectChildren to filter some of them. This is a bit fragile nowadays since when we search all targets we don't use that definition.
It's a bit of a tangential feature, but it would be nice to have a dynamic def enabled: Boolean in modules that is supported and works correctly all the time, (another possible candidate is def bspEnabled to disable the IDE features for some modules and also speedup builds). Once we have that, we could also think to use the dynamic approach in Mill itself.

By the way, I will review the PR later :)

@lihaoyi
Copy link
Member Author

lihaoyi commented Apr 3, 2023

@lolgab looking at your repo, it seems your implementation has the advantage of adding a CrossPlatform for additional convenience (e.g. shared moduleDeps) whereas this PR uses a raw mill.Module. It also has the Disabling platforms dynamically ability.

I can try porting over that functionality into com-lihaoyi/mill - seems like a relatively small amount of code - and update the example accordingly

@lihaoyi
Copy link
Member Author

lihaoyi commented Apr 4, 2023

I find the Wrapper trait name hard to grasp. Maybe we can find some better name for it? And in the example we should definitely use some example domain name instead of wrapper, to make sure, the names are not bound. (They are just coincidentally named "wrapper".)

Yeah I don't like the name either, but I don't know what else to call it. Base? Root? Basically we want to do the cross-version stuff once relatively high up in the module tree, so that we don't need to repeat that stuff over and over for every module

@lihaoyi
Copy link
Member Author

lihaoyi commented Apr 7, 2023

If there's no other feedback, I'm planning on merging this tomorrow. This is a relatively spartan implementation, and somewhat different from https://github.com/lolgab/mill-crossplatform, but I think provides enough value as is. We can continue to refine the code in main if there are further improvements we would like to make

@lefou
Copy link
Member

lefou commented Apr 7, 2023

@lihaoyi Can we find a better name for the Wrapper trait?

I'm heavily time-pressured right now and your lately pushing, although very appreciated, is forcing me into reaction mode. I find it hard to come up with some better name, but Wrapper is too generic and transports exactly zero meaning about its purpose. This is API code which we need to maintain, and binary compatibility is a hard to deal with issue in Mill, even if there are some @experimental or @internal annotations. We should not rush, as changing it later is more difficult than thinking it through now.

I guess any other proposal containing the words "cross", "base" or "template" in it are already better that "wrapper".

@lihaoyi
Copy link
Member Author

lihaoyi commented Apr 7, 2023

@lefou how about CrossScalaModule.Base?

@lefou
Copy link
Member

lefou commented Apr 7, 2023

@lefou how about CrossScalaModule.Base?

That will work.

@lihaoyi
Copy link
Member Author

lihaoyi commented Apr 8, 2023

Updated the name, will probably merge tomorrow if CI is happy

@lihaoyi lihaoyi merged commit 785e878 into com-lihaoyi:main Apr 8, 2023
29 of 30 checks passed
@lefou lefou added this to the 0.11.0-M8 milestone Apr 10, 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

Successfully merging this pull request may close these issues.

Simplify the setup for cross-platform cross-version Scala modules
3 participants