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

Remove Ammonite as a dependency, handle script running and bootstrapping ourselves #2377

Merged
merged 208 commits into from Apr 2, 2023

Conversation

lihaoyi
Copy link
Member

@lihaoyi lihaoyi commented Mar 23, 2023

The basic idea is that instead of relying on Ammonite to evaluate the build.sc and return us a mill.define.BaseModule value, we instead

  1. First instantiate an in-memory MillBuildRootModule, which is a special ScalaModule configured to read build.sc and transform it into Build.scala before compiling it with the necessary classpath

  2. Use a mill.eval.Evaluator to call runClasspath on the MillBootstrapModule, which gives us a list of files and folders that are necessary to use it

  3. Put the files and folders into a java.net.URLClassloader to load it into the JVM, and use java reflection to fish out the final mill.define.BaseModule runtime value

These are all largely things that were already happening before, just inside the guts of the Ammonite script runner, and mixed in with a bunch of other Ammonite REPL stuff we don't want, and via parallel code paths (e.g. ammonite.compiler.Compiler vs mill.scalalib.worker.ZincWorker). By pulling them out, we can re-use the bulk of ScalaModule logic we already have, and just need to do the minimal amount of classloader plumbing to get us what we want

Major Code Changes

  1. In order to let us depend on ScalaModule within MillMain, we need to split out MillMain from the rest of main/ to avoid a circular dependency with scalalib/. For now I dumped them in a runner/ module that depends on both main/ and scalalib/, along with similar classes like BspContext.scala, GraphUtils.scala, etc.

  2. Vendored/adapted a bunch of Ammonite code: Parsers.scala, Classpath.scala, Colors.scala, class AnsiNav, Watchable.scala, LineNumberPlugin.scala, Util.scala

  3. The old MillMain/MainRunner/RunScript logic has been reorganized into MillMain/MillBuildBootstrap/MillBuildModule:

    a. MillBuildModule is a hard-coded build.sc file. This module knows how to parse the build.sc, walk its import $file/$ivy, and generate the relevant generatedSources/ivyDeps for it to be compiled as a normal ScalaModule with the appropriate compiler-plugins/scala-version/etc.

    b. MillBuildBootstrap performs the main bootstrapping logic: creating a MillBootModule, evaluating its runClasspath, loading the classfiles into a classloader, grabbing the resultant BaseModule from that classloader and then evaluating the targets that the user asked for

    c. MillMain is just the program entrypoint, mostly unchanged except we replace using of MainRunner with MillBootstrap

  4. Removed the mill -i build REPL. We can consider adding it back in future if necessary, but for now the last poll basically indicated it is not used and it's not really well maintained.

  5. Removed all Ammonite dependencies from Mill's own build.sc

  6. EvaluatorState is renamed to RunnerState, and now contains a list of Frames each one representing the output of one level of the meta-build

  7. Top-level modules are now supported: simply define a module at the top level of your build.sc with extends mill.runner.BaseModule, and it will be treated as the top-level module for resolving tasks and submodules. We make use of this for supporting meta-builds e.g. mill-build/build.sc

Smaller Code Changes

  1. Made PathRef#toString use the same string as upickle.default.write(pathRef)

  2. Consolidated all the stdin/stdout/stderr triples into one SystemStreams class to make them easier to pass around

  3. Added a bunch more explicit dependencies that Mill was previously inheriting from Ammonite: coursier, requests, scalaparse etc.

  4. Consolidated a bunch of custom implicit defs with import mill.main.TokenReaders._

  5. Added an --enable-ticker <bool> flag, which unlike --disable-ticker can be passed true or false. We now automatically enable or disable the ticker based on mainInteractive, which is probably what most people want, and you can use --enable-ticker <bool> to override it either way

  6. Simplified the computation of script dependency-based invalidation, now just using file-paths rather than class names and avoiding the confusing conversions back and forth

  7. We dump the RunnerState.Frame to disk after every MillBuildBootstrap.evaluate call, to support both automated testing as well as manual debugging when things go wrong.

Testing

  1. All existing tests in the test suite pass

  2. I moved all ScriptTestSuites that rely on MillMain into the integration/ test folder, since now they rely not just on the stuff in main/ but also the stuff in runner/

  3. Made the bspWorkerLibs task use our standard way of choosing between local classpath and ivy deps, to allow it to be tested locally without needing to publishLocal

  4. I added a few new tests:

    a. MultiLevelBuildTests, which takes a 3-level build (build.sc, mill-build/build.sc, mill-build/mill-build/build.sc) and walks through making changes (valid, parse error, compile error, runtime error) to various levels of the project and ensuring that the output/error, watched files, classloader invalidation, etc. all works as expected

    b. ParseErrorTests and CompileErrorTests, to make sure the messages are shown properly with proper line/col numbers

    c. TopLevelModuleTests, to exercise the new top-leval module functionality

  5. I modified the integration.local tests to persist the RunnerState between command invocations, so now it behaves more like integration.server than integration.forked

  6. Made contrib/scoverage/test/src/mill/contrib/scoverage/HelloWorldTests.scala tests pass on repos not named mill (so it can pass when run on github.com/lihaoyi/mill-1

  7. Consolidated ScriptTestSuite into IntegrationTestSuite and standardized all test suites on using that trait

TODO

  1. Some story around how the MillBuildModule can share things with the normal build.sc modules. Do they get cleaned together? Can MillBuildModule be inspected from the CLI? Can they share their ZincWorker?

  2. Can we unify the mill.runner.MillBuildModule with mill.scalalib.bsp.MillBuildModule?

…nc happy, fixes mill.integration.forked.DocAnnotationsTests.test
@lihaoyi lihaoyi merged commit 5998b56 into com-lihaoyi:main Apr 2, 2023
31 checks passed
lihaoyi added a commit that referenced this pull request Apr 29, 2023
…h management to allow testing (#2476)

Fixes #2474

These were overlooked in #2377
and deleted, and were not covered by any tests. But it's straightforward
to put them back.

# Major Changes

1. Put back `MillIvy.scala` and call it in `MillBuildRootModule.scala`

# Testing 

1. I added an example test `example/misc/6-contrib-import` to both
exercise the code as well as serve as an example we can include in our
docs

2. In order to allow usage of contrib module in `.local` integration and
example tests, I moved the handling of Mill test classpath overrides
from `Util.millProjectModule` to `CoursierSupport#resolveDependencies`.
This lets us be more override contrib modules dependency resolution,
even though they don't have a neat single location for us to call our
`millProjectModule` helper.

3. I refactored `millProjectModule` to not need a `key`, so we just
compute the key based on the dependency name, keeping them consistent
and removing an unnecessary degree of freedom

4. The local-testing-classpath-overrides were moved from using JVM
system properties to instead use classpath resources: we look for
overrides in `mill/local-test-overrides/*`. This should remove any
security worries:
1. Before you only needed to modify the JVM props or `JAVA_OPTS`
environment variable, and could replace the code of a Mill module to any
local filesystem path
2. Now, you need access to modify the Mill classpath to trigger the test
overrides, and at that point you already have access to modifying the
Mill classfiles being executed anyway

While it probably was not strictly necessary to clean up the test
classpath overrides logic as part of this PR, the status quo in master
was a pile of hacks, and I didn't feel like adding another hack to get
`.local` testability of contrib libraries working. With these changes,
contrib libraries work the same as the existing Mill test classpath
overrides, and things are cleaned up so much that the net lines of code
for this PR is negative

There's still more cleanup to do in `build.sc`, but those can come in
follow up PRs
lihaoyi added a commit that referenced this pull request May 5, 2023
Fixes #2485

We take the `config.imports` field and pass it to
`MillIvy.processMillIvyDepSignature` inside `ivyDeps`. This was
overlooked in #2377, but seems
easy enough to put back

Honestly not super sure how to test this. But when I run

```
./mill -i dev.run example/tasks/3-anonymous-tasks -i --import ivy:com.lihaoyi::mill-contrib-bloop:0.11.0-M8  mill.contrib.bloop.Bloop/install
```

On main branch, I get 

```
Cannot resolve external module mill.contrib.bloop.Bloop
```

On this PR I get

```
java.lang.NoSuchMethodError: 'java.lang.ThreadLocal mill.eval.Evaluator$.currentEvaluator()'
    mill.contrib.bloop.Bloop$$anonfun$$lessinit$greater$1.apply(Bloop.scala:8)
    mill.contrib.bloop.Bloop$$anonfun$$lessinit$greater$1.apply(Bloop.scala:8)
```

Seems like there's a binary incompatibility, but at least the `--import`
flag took effect?
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.

None yet

3 participants