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 an example doc folder for writing and testing a Mill plugin #3416

Merged
merged 25 commits into from
Aug 25, 2024

Conversation

lihaoyi
Copy link
Member

@lihaoyi lihaoyi commented Aug 24, 2024

Most of the interesting stuff is in example/extending/plugins/7-writing-mill-plugins/build.sc. It's a simple example build for a Mill plugin that has unit, integration, and example tests

I did some unrelated refactoring as part of this PR, to try and reduce the messiness of the example folder and make it better match the current docsite, and some cleanup in the build.sc that was necessary to make the example test pass (e.g. dev.* is now dist.*)

@lihaoyi lihaoyi requested a review from lefou August 24, 2024 14:44
@lihaoyi
Copy link
Member Author

lihaoyi commented Aug 24, 2024

This is still WIP but it could use a first pass review

@lihaoyi lihaoyi marked this pull request as ready for review August 25, 2024 01:40
@lihaoyi lihaoyi changed the title [WIP] Add an example doc folder for writing and testing a Mill plugin Add an example doc folder for writing and testing a Mill plugin Aug 25, 2024
@lihaoyi lihaoyi merged commit dbdcbf6 into com-lihaoyi:main Aug 25, 2024
33 checks passed
@lihaoyi
Copy link
Member Author

lihaoyi commented Aug 25, 2024

Merging this first to avoid the PR snowballing larger, @lefou feel free to review example/extending/plugins/7-writing-mill-plugins/build.sc and we can continue to adjust the example in follow ups

@lefou
Copy link
Member

lefou commented Aug 25, 2024

Can't review in detail right now, but writing down some ad-hoc thoughts.

As for any other project, Mill plugins should declare minimal dependencies. Depending on mill-dist is IMHO way too much. E.g. plugins only defining modules (like mill-vcs-version) can just depend on mill-main-api, many depend on mill-scalalib (e.g. mill-kotlin).

They should also depend on the lowest version they want to support, this is typically the {mill-binary-verion}.0 version. This is to give users the choice to not run with the latest version, for whatever reasons.

To avoid bringing incompatible or duplicate entries to the classpath, they should declare Mill dependencies as compileIvyDeps which translate to Maven provided scope. The actual versions will be provided by the actual running Mill. This is likely a best practice for any "plugin"-project in relation to their host APIs.

The integration testing should IMHO also happen against multiple Mill versions. At least the minimal version and ideally more recent versions.

And since we try to evolve a stable environment, it's a good idea to prepare a Mill plugin project for cross-compilation against multiple Mill versions from the beginning. It's always a bit harder to add that later. When there is a nice and simple template which prepares that, it makes later support for multiple versions easier. Why is that necessary? Because a good build tool needs to address not only small hobby projects, but larger commercial projects that have various reasons to not upgrade their tooling any other day. Also, you can't make large changes in maintenance branches.

Also, we already use the mill-dev artifact name in launcher scripts, so just renaming it to mill-dist, although being the better name, isn't a good idea. At least not in the middle of binary compatible evolution. It would immediately break any attempt to use a newer version.

@lefou lefou added this to the 0.12.0 milestone Aug 25, 2024
@lihaoyi
Copy link
Member Author

lihaoyi commented Aug 25, 2024

As for any other project, Mill plugins should declare minimal dependencies. Depending on mill-dist is IMHO way too much. E.g. plugins only defining modules (like mill-vcs-version) can just depend on mill-main-api, many depend on mill-scalalib (e.g. mill-kotlin).

They should also depend on the lowest version they want to support, this is typically the {mill-binary-verion}.0 version. This is to give users the choice to not run with the latest version, for whatever reasons.

To avoid bringing incompatible or duplicate entries to the classpath, they should declare Mill dependencies as compileIvyDeps which translate to Maven provided scope. The actual versions will be provided by the actual running Mill. This is likely a best practice for any "plugin"-project in relation to their host APIs.

Does this really matter since Mill is always distributed as mill-dist anyway? Like AFAIK whatever versions of the various jars people pull in doesn't matter, since the classloader hierarchy will always pull in the version from the enclosing Mill executable. I don't remember for sure, but don't we also filter out Mill dependencies somewhere in Mill's own bootstrapping process? I vaguely thought I saw code related to that somewhere but I'm not sure

The integration testing should IMHO also happen against multiple Mill versions. At least the minimal version and ideally more recent versions.

Can probably be done. Not super familiar with Mill cross versioning, so @lefou you may need to be the one adding it here. Like do we have to add _mill0.11 suffixes or something here?

Also, we already use the mill-dev artifact name in launcher scripts, so just renaming it to mill-dist, although being the better name, isn't a good idea. At least not in the middle of binary compatible evolution. It would immediately break any attempt to use a newer version.

AFAIK we only use mill-dist in launcher scripts. At least that's what's in the ./mill launcher script in com-lihaoyi/mill right now. dev seems to only be used in Mill development workflows (dev.run, dev.assembly, dev.launcher), so given both dev and dist kind of do the same thing and I wanted to consolidate them, I chose to consolidate them to the name that's used externally even if that means the internal workflows will have to change

@lefou
Copy link
Member

lefou commented Aug 25, 2024

As for any other project, Mill plugins should declare minimal dependencies. Depending on mill-dist is IMHO way too much. E.g. plugins only defining modules (like mill-vcs-version) can just depend on mill-main-api, many depend on mill-scalalib (e.g. mill-kotlin).

They should also depend on the lowest version they want to support, this is typically the {mill-binary-verion}.0 version. This is to give users the choice to not run with the latest version, for whatever reasons.

To avoid bringing incompatible or duplicate entries to the classpath, they should declare Mill dependencies as compileIvyDeps which translate to Maven provided scope. The actual versions will be provided by the actual running Mill. This is likely a best practice for any "plugin"-project in relation to their host APIs.

Does this really matter since Mill is always distributed as mill-dist anyway? Like AFAIK whatever versions of the various jars people pull in doesn't matter, since the classloader hierarchy will always pull in the version from the enclosing Mill executable. I don't remember for sure, but don't we also filter out Mill dependencies somewhere in Mill's own bootstrapping process? I vaguely thought I saw code related to that somewhere but I'm not sure

Yes, it matters.

  • Some plugins are dependencies of other plugins/tools, e.g. mill-vcs-version is required by mill-ci-release
  • Non API classes in classpath encourage users to use them, making their project eligible to binary compatibility issues
  • larger classpaths might be slower, need more time (to transfer) and space (to store)
  • mill-dist is still an assembly containing other third-party libs (including com.lihaoyi ones), which are transparent to any dependency resolver and can cause unforeseen trouble

About the classpath filtering. We don't have an effective way currently. We tried to hack something to mitigate some issues due to mill bootstrap builds using a unmanagedClasspath, but it's far from accurate for all cases. But we plan to change to bootstrap build anyways, which brings me to the next point.

Once we change details in how we build mill project files we risk to run into issue. E.g. if we expect less dependencies on the classpath, they can still slip in via too coarsely defined transitive dependencies.

The integration testing should IMHO also happen against multiple Mill versions. At least the minimal version and ideally more recent versions.

Can probably be done. Not super familiar with Mill cross versioning, so @lefou you may need to be the one adding it here. Like do we have to add _mill0.11 suffixes or something here?

The _mill0.11 should be added in any case, since we need to make sure plugins don't get loaded into an binary incompatible Mill runtime. We should define the the platformPrefix, then we can use the platform notation.

Beside that, it's mostly the Cross[_] ceremony we could prepare, and potential potential differences in the artifacts required for each Mill version. I think we should try to provide a MillPluginModule, so that we could abstract away the details and make it easier for users.

Also, we already use the mill-dev artifact name in launcher scripts, so just renaming it to mill-dist, although being the better name, isn't a good idea. At least not in the middle of binary compatible evolution. It would immediately break any attempt to use a newer version.

AFAIK we only use mill-dist in launcher scripts. At least that's what's in the ./mill launcher script in com-lihaoyi/mill right now. dev seems to only be used in Mill development workflows (dev.run, dev.assembly, dev.launcher), so given both dev and dist kind of do the same thing and I wanted to consolidate them, I chose to consolidate them to the name that's used externally even if that means the internal workflows will have to change

millw is also using it, to download Mill assembly from Maven Central. It's probably easy enough to match based on the version. We're early in the 0.12 phase and no stable releases are out, so we can just remember users to update their scripts in the release notes. We need to address this in millw of course, which has many users, esp. those on Windows and via Metals.

@lefou
Copy link
Member

lefou commented Aug 26, 2024

Also, we already use the mill-dev artifact name in launcher scripts, so just renaming it to mill-dist, although being the better name, isn't a good idea. At least not in the middle of binary compatible evolution. It would immediately break any attempt to use a newer version.

AFAIK we only use mill-dist in launcher scripts. At least that's what's in the ./mill launcher script in com-lihaoyi/mill right now. dev seems to only be used in Mill development workflows (dev.run, dev.assembly, dev.launcher), so given both dev and dist kind of do the same thing and I wanted to consolidate them, I chose to consolidate them to the name that's used externally even if that means the internal workflows will have to change

millw is also using it, to download Mill assembly from Maven Central. It's probably easy enough to match based on the version. We're early in the 0.12 phase and no stable releases are out, so we can just remember users to update their scripts in the release notes. We need to address this in millw of course, which has many users, esp. those on Windows and via Metals.

Oh, I totally missed, that mill-dist (not mill-dev) was already the artifact ID we publish and use. Please ignore all my statement about the dev -> dist migration. There are also no open TODOs in millw. I'm sorry for the confusion.

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.

2 participants