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

Fix issue #2958 - deduplicate (anonymous) tasks in result #2959

Merged
merged 6 commits into from
Jan 10, 2024
Merged

Conversation

lefou
Copy link
Member

@lefou lefou commented Jan 8, 2024

Anonymous tasks can be defined as val or def. This is in contrast to named targets, which always need to be defined as def.

Also, anonymous tasks end up in the terminal group of the target that's using them.

If the same anonymous task is used in multiple targets, it also ends up multiple time in the task results. When a Evaluator.Results is created, this clashes, due to the use of a Strict.Agg for Evalutor.Results.evaluated.

This change deduplicated the results before constructing the Evaluator.Results.

Fix: #2958

Ensure when flatMap-ing, that we don't append duplicates.
Whether this is correct needs yet to be analyzed, but in general,
we don't enfore unique-ness of task results, and even when we evaluate
disjuct tasks, we can't assume that all results are disjuct, hence
using a strict Agg, which enforces uniqueness on append seems not
correct.

Here's my reasoning: If we have two tasks A and B, and B just forwards
the result of A as it's own result, both tasks return the same result.
Comment on lines 172 to 177
Strict.Agg.from(
Loose.Agg.from(
finishedOptsMap.values.flatMap(_.toSeq.flatMap(_.newEvaluated))
)
),
transitive,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So as far as I can tell, evaluated: Agg[Task[_]] is only used in the test suite, and transitive: Agg[Task[_]] is not used at all. So this should be fine from a runtime behavior perspective. And anyway it's private[mill] so we change things without worrying about compatibility.

As for what the correct thing to do here is, Strict.Agg.from(Loose.Agg.from(can just be a single.distinct` call I think. Then maybe just a comment and a simple test case illustrating the edge case and then we should be good to go

@lefou lefou changed the title Quick fix for issue #2958 Fix issue #2958 - duplicate (anonymous) tasks in result Jan 9, 2024
@lefou lefou marked this pull request as ready for review January 9, 2024 14:34
@lefou lefou changed the title Fix issue #2958 - duplicate (anonymous) tasks in result Fix issue #2958 - deduplicate (anonymous) tasks in result Jan 9, 2024
@lefou
Copy link
Member Author

lefou commented Jan 9, 2024

My new test fails on Windows, but I have no idea why.

[#1] [#1] [#3] [#00] mill.eval.SeqTaskTests: [1/3] repro2958.task2 
[#1] [#1] [#3] [#00] mill.eval.SeqTaskTests: [2/3] repro2958.task3 
[#1] [#1] [#3] [#00] mill.eval.SeqTaskTests: [3/3] repro2958.com1 
X mill.eval.SeqTaskTests.duplicateTaskInResult-issue2958 4ms 
  java.nio.file.NoSuchFileException: D:\a\mill\mill\target\workspace\mill\eval\SeqTaskTests\check\duplicateTaskInResult-issue2958\SeqTaskTests$\repro2958\com1.json
    sun.nio.fs.WindowsException.translateToIOException(WindowsException.java:79)
    sun.nio.fs.WindowsException.rethrowAsIOException(WindowsException.java:97)
    sun.nio.fs.WindowsException.rethrowAsIOException(WindowsException.java:102)
    sun.nio.fs.WindowsFileSystemProvider.newByteChannel(WindowsFileSystemProvider.java:230)
    java.nio.file.Files.newByteChannel(Files.java:361)
    os.write$.write(ReadWriteOps.scala:64)
    os.write$over$.apply(ReadWriteOps.scala:142)
    mill.eval.GroupEvaluator.$anonfun$handleTaskResult$2(GroupEvaluator.scala:406)
    mill.eval.GroupEvaluator.$anonfun$handleTaskResult$2$adapted(GroupEvaluator.scala:405)
    scala.Option.foreach(Option.scala:437)
    mill.eval.GroupEvaluator.handleTaskResult(GroupEvaluator.scala:405)
    mill.eval.GroupEvaluator.evaluateGroupCached(GroupEvaluator.scala:226)
    mill.eval.GroupEvaluator.evaluateGroupCached$(GroupEvaluator.scala:45)
    mill.eval.EvaluatorImpl.evaluateGroupCached(EvaluatorImpl.scala:15)
    mill.eval.EvaluatorCore.$anonfun$evaluate0$2(EvaluatorCore.scala:116)
    scala.concurrent.impl.Promise$Transformation.run(Promise.scala:467)
    mill.eval.ExecutionContexts$RunNow$.execute(ExecutionContexts.scala:13)
    scala.concurrent.impl.Promise$Transformation.submitWithValue(Promise.scala:429)
    scala.concurrent.impl.Promise$DefaultPromise.submitWithValue(Promise.scala:338)
    scala.concurrent.impl.Promise$DefaultPromise.dispatchOrAddCallbacks(Promise.scala:312)
    scala.concurrent.impl.Promise$DefaultPromise.map(Promise.scala:182)
    mill.eval.EvaluatorCore.$anonfun$evaluate0$1(EvaluatorCore.scala:92)
    mill.eval.EvaluatorCore.$anonfun$evaluate0$1$adapted(EvaluatorCore.scala:90)
    scala.collection.immutable.Vector.foreach(Vector.scala:2124)
    mill.eval.EvaluatorCore.evaluate0(EvaluatorCore.scala:90)
    mill.eval.EvaluatorCore.$anonfun$evaluate$1(EvaluatorCore.scala:43)
    scala.util.DynamicVariable.withValue(DynamicVariable.scala:59)
    mill.eval.EvaluatorCore.evaluate(EvaluatorCore.scala:34)
    mill.eval.EvaluatorCore.evaluate$(EvaluatorCore.scala:26)
    mill.eval.EvaluatorImpl.evaluate(EvaluatorImpl.scala:15)
    mill.testkit.MillTestKit$TestEvaluator.apply(MillTestkit.scala:128)
    mill.testkit.MillTestKit$TestEvaluator.apply(MillTestkit.scala:121)
    mill.eval.TaskTests.$anonfun$tests$34(TaskTests.scala:257)
    mill.eval.TaskTests.$anonfun$tests$34$adapted(TaskTests.scala:256)
    mill.eval.SeqTaskTests$.withEnv(TaskTests.scala:271)
    mill.eval.TaskTests.$anonfun$tests$33(TaskTests.scala:137)

@lihaoyi
Copy link
Member

lihaoyi commented Jan 10, 2024

@lefou is it because your target starts with the word com which is prohibited on the windows filesystem?

@lefou
Copy link
Member Author

lefou commented Jan 10, 2024

@lefou is it because your target starts with the word com which is prohibited on the windows filesystem?

Thanks for the hint. In fact, it's the exact name "com1" that is reserved, although I wonder why the file name extension isn't counted as part of the name.

There are other reserved names, which we should try to detect and handle somehow in Mill, to avoid misleading error messages: CON, PRN, AUX, NUL, COM0, COM1, COM2, COM3, COM4, COM5, COM6, COM7, COM8, COM9, COM¹, COM², COM³, LPT0, LPT1, LPT2, LPT3, LPT4, LPT5, LPT6, LPT7, LPT8, LPT9, LPT¹, LPT², and LPT³

@lefou lefou merged commit 8eb07d7 into main Jan 10, 2024
38 checks passed
@lefou lefou deleted the quickfix-2958 branch January 10, 2024 10:26
@lefou lefou added this to the 0.11.7 milestone Jan 10, 2024
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.

Repro for Duplicated item inserted into OrderedSet
2 participants