-
-
Notifications
You must be signed in to change notification settings - Fork 360
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
More refactoring for Resolve logic #2511
Conversation
resolve
from main
into its own moduleThere 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.
This looks good to me. I think, we should provide some @deprecated
forwarders for the classes moved to mill.resolve
in the mill.main
package object. This should improve migration convenience and cross compilation.
@lihaoyi We have one failing test in main and it seems related to this PR. Could you please look at it?
|
will take a look |
CI is green now, but I do not understand what caused the breakage, nor whay fixed it, so we'll have to see if the issue reappears. If it turns out the test is just flaky then we may be forced to disable it |
1 similar comment
CI is green now, but I do not understand what caused the breakage, nor whay fixed it, so we'll have to see if the issue reappears. If it turns out the test is just flaky then we may be forced to disable it |
This PR add support for new selector pattern `_:Type`. In addition to `_` and `__`, which select arbitrary segments, the `_:MyType` and `__:MyType` patterns can select modules of the specified type. The type is matched by it's name and optionally by it's enclosing types and packages, separated by a `.` sign. Since this is also used to separate target path segments, a type selector segment containing a `.` needs to be enclosed in parenthesis. A full qualified type can be enforced with the `_root_` package. Example: Find all test jars ```sh > mill resolve __:TestModule.jar > mill resolve "(__:scalalib.TestModule).jar" > mill resolve "(__:mill.scalalib.TestModule).jar" > mill resolve "(__:_root_.mill.scalalib.TestModule).jar" ``` If a `^` or `!` is preceding the type pattern, it only matches segments not an instance of that specified type. Please note that in some shells like `bash`, you need to mask the `!` character. Example: Find all jars not in test modules ```sh > mill resolve __:^TestModule.jar ``` You can also provide more than one type pattern, separated with `:`. Example: Find all `JavaModule`s which are not `ScalaModule`s or `TestModule`s: ```sh > mill resolve "__:JavaModule:^ScalaModule:^TestModule.jar" ``` Remarks: * Kudos to @lihaoyi who refactored the resolver in #2511 and made this PR possible. I tried to implement it multiple times before, and ever got bitten by the old gnarly resolver code. * It's currently not possible to match task/target types. It might be possible, but due to `Task` being a parametrized type, it might not be as easy to implement and use. Fix #1550 Pull request: #2997
Major Changes
Break out
main/resolve/
frommain/
, depending onmain/define/
but not depending onmain/eval/
.Evaluator
, and the core logic ofEvaluator
should not depend on task resolution (even if individual tasks might require it).Minimize the amount of actual instantiations we perform in
ResolveCore
Target
s andCommand
s until resolution is complete, removing a whole bunch of messymainargs
logic for the already-messy recursive resolution code. A lot of it ends up inResolve.scala
, which is somewhat more straightforward and better able to absorb the messinessModule
s when trying to resolve tasks within aCross
module, since that requires us to instantiate theCross
object to see it's cross keys. Most resolution can take place purely by reflecting onjava.lang.Class[_]
s without needing the modules to actually be instantiatedResolveCore
now only returns thesegments: Segments
for the things it resolves, leaving instantiation toResolve.Tasks
ResolveCore
, lettingmill resolve
work faster and work even when the modules cannot instantiate, unless there is aCross
module we need to instantiateRefactored
ResolveNonEmpty
toResolveNotFoundHandler
, to only be called byResolve
ifResolveCore
returns aResolveCore.NotFound
. That helps avoid deep call hierarchies, and allows us to ensure thatResolveCore
andResolveNotFoundHandler
independent of each other (rather than one wrapping the other)Extracted
mill.define.Module.Internal
into a separatemill.define.Reflect
object. This helps clean upModule.scala
. Ideally I wanted to moveReflect
tomill.resolve
, but it's difficult to do that while still providing nice helper methods onmill.define.Module
objects e.g.millModuleDirectChildren
while allowing them to be overriden etc.ResolveTasks
is nowResolve.Tasks
,ResolveSegments
is nowResolve.Segments
,ResolveMetadata
is removed sinceResolve.Segments
does more or less the same thing anywayMake
Cross#items
aList[Item]
, so we can publicize it while continuing to evolve it in a binary-compatible mannerTesting
Added more asserts to the existing
ResolveTests.scala
unit tests to check thatResolveSegments
can still succeed even whenResolveTasks
fails due to module initialization errorsAdded additional test cases to check the behavior of cross-module failures in more scenarios: when
_
and__
is used, when the cross module itself fails to initialize (previously we only checked for children failing to initialize), and when the cross module's parent module fails to initializeAdded a few test cases to
integration/failure/module-init-error/
exercise the "resolve works even when module cannot initialize" behavior end-to-endExisting test cases in
ResolveTests.scala
and downstream tests that exercise task resolution all pass