-
-
Notifications
You must be signed in to change notification settings - Fork 311
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
Improve overriden target detection to let it handle stackable-traits-style overrides #1600
Conversation
I love that we get rid of the |
@lefou looks like the bootstrap tests are failing due to some external plugins. Not sure what your SOP for dealing with these is, but IMO for plugins Mill's own build depends on we should just keep them in the mill repo's contrib folder to simplify this sort of change |
Bootstrap tests are special and can break when we change some API of Mill. Although you see it fail because of plugins, this issue can also happen when we don't depend on external plugins, but instead use some API that has changed directly. (I remember we had that issue in the past, e.g. when we changed some publishing API.) Our CI now applies a patch file before it runs bootstrap tests, it's I don't think we should deny ourselves to use external plugins, just because we want to run some bootstrap tests. Instead, we can adapt the build (and ad-hoc disable plugins) to run the bootstrap tests. https://github.com/com-lihaoyi/mill/commits/main/ci/mill-bootstrap.patch |
I can prepare a patch file and push it to your branch later. I typically edit the project and then run
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Resolves #598. Input tasks results are re-evaluated every time and thus never read from the disk, and so a `upickle.default.Reader` is not required. Nevertheless, we still require that a `upickle.default.Writer` in order to generate the `*.json` files used for `./mill show` and other debugging purposes. Targeting #1600 to avoid merge conflicts Let's see what CI says... TBH not sure if this is worth doing, or whether we should just specify that `Input` tasks require a `ReadWriter` just to keep things consistent. We don't currently have any use case for de-serializing the input task JSON (e.g. even `mill show` takes the JSON from disk and spits it out verbatim without de-serializing) but that doesn't mean we won't find such use cases in future. But if such use cases do re-appear, we can always add that functionality back. `Command`s are write-only, so making `Input`s write-only would also not be unprecedented
…style overrides (com-lihaoyi#1600) Fixes com-lihaoyi#554 This PR swaps out our detection of overriden targets and commands: - Previously, we tried to count how many overriden methods there are upstream of a particular target, statically at its definition site. Any target that has the max number of overriden methods is considered the "final" one, while others are considered overriden. This fails for stackable traits, e.g. the example in the test suite below, as multiple traits can have the same number of static definiton-site overrides but when stacked together on a module one of them ends up winning - Now, we simply do a reverse walk of the targets list after they have been topo sorted. Since the directionality of overrides is exactly opposite that of the topo sort, we can assume that the first time we see a target of a particular label in the reverse walk it's the "final" one, while any subsequent targets we see with the same label must be the overriden versions The new mechanism is simpler, and possibly more correct. Hopefully CI would catch any issues... Added a test case that fails on master and passes on this PR. Also verified manually that the issue reported above can be reproduced on master, and does not reproduce (i.e. it behaves correctly) on this PR.
…1601) Resolves com-lihaoyi#598. Input tasks results are re-evaluated every time and thus never read from the disk, and so a `upickle.default.Reader` is not required. Nevertheless, we still require that a `upickle.default.Writer` in order to generate the `*.json` files used for `./mill show` and other debugging purposes. Targeting com-lihaoyi#1600 to avoid merge conflicts Let's see what CI says... TBH not sure if this is worth doing, or whether we should just specify that `Input` tasks require a `ReadWriter` just to keep things consistent. We don't currently have any use case for de-serializing the input task JSON (e.g. even `mill show` takes the JSON from disk and spits it out verbatim without de-serializing) but that doesn't mean we won't find such use cases in future. But if such use cases do re-appear, we can always add that functionality back. `Command`s are write-only, so making `Input`s write-only would also not be unprecedented
Fixes #2198 There's a bit of duplication if a user both adds `@mainargs.arg(doc = "...")` scala annotation as well as a `@param foo` scaladoc annotation, but that's an inherent issue with mainargs and not something we aim to fix here. We render the mainargs CLI help message the same way it's done in mainargs. There isn't a nice helper that renders the whole thing at once, so i have to piece it together from various helpers, but it's not too complex and works out ok. Tested via additions to `integration.feature[docannotations].local.test`, and manually ```bash $ ./mill -i dev.run example/basic/1-simple-scala -i inspect run # command with 1 arg run(JavaModule.scala:720) Runs this modules code in a subprocess and waits for it to finish args <str>... Inputs: finalMainClass runClasspath forkArgs forkEnv forkWorkingDir runUseArgsFile $ ./mill -i dev.run example/basic/1-simple-scala -i inspect compile # not a command compile(ScalaModule.scala:211) Compiles the current module to generate compiled classfiles/bytecode. When you override this, you probably also want to override [[bspCompileClassesPath]]. Inputs: scalaVersion upstreamCompileOutput allSourceFiles compileClasspath javacOptions scalaOrganization allScalacOptions scalaCompilerClasspath scalacPluginClasspath zincReportCachedProblems $ ./mill -i dev.run example/basic/1-simple-scala -i inspect console # command with no args console(ScalaModule.scala:369) Opens up a Scala console with your module and all dependencies present, for you to test and operate your code interactively. Inputs: scalaVersion runClasspath scalaCompilerClasspath forkArgs forkEnv forkWorkingDir $ ./mill -i dev.run example/basic/1-simple-scala -i inspect ivyDepsTree # command with lots of args ivyDepsTree(JavaModule.scala:688) Command to print the transitive dependency tree to STDOUT. --inverse Invert the tree representation, so that the root is on the bottom val inverse (will be forced when used with whatDependsOn) --whatDependsOn <str> Possible list of modules (org:artifact) to target in the tree in order to see where a dependency stems from. --withCompile Include the compile-time only dependencies (`compileIvyDeps`, provided scope) into the tree. --withRuntime Include the runtime dependencies (`runIvyDeps`, runtime scope) into the tree. Inputs: transitiveIvyDeps scalaVersion scalaVersion scalaOrganization scalaVersion ``` I also removed the `numOverrides` field on `mill.define.Discover`, since it's unused since #1600 landed a while back
Fixes #554
This PR swaps out our detection of overriden targets and commands:
Previously, we tried to count how many overriden methods there are upstream of a particular target, statically at its definition site. Any target that has the max number of overriden methods is considered the "final" one, while others are considered overriden. This fails for stackable traits, e.g. the example in the test suite below, as multiple traits can have the same number of static definiton-site overrides but when stacked together on a module one of them ends up winning
Now, we simply do a reverse walk of the targets list after they have been topo sorted. Since the directionality of overrides is exactly opposite that of the topo sort, we can assume that the first time we see a target of a particular label in the reverse walk it's the "final" one, while any subsequent targets we see with the same label must be the overriden versions
The new mechanism is simpler, and possibly more correct. Hopefully CI would catch any issues...
Added a test case that fails on master and passes on this PR. Also verified manually that the issue reported above can be reproduced on master, and does not reproduce (i.e. it behaves correctly) on this PR.