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

Revert #1284 Include mutual dependency in initial invalidation #1462

Merged
merged 2 commits into from
Oct 17, 2024

Conversation

lihaoyi
Copy link
Contributor

@lihaoyi lihaoyi commented Oct 16, 2024

Ref #1284
Fixes #1461
Fixes #1420
Also fixes com-lihaoyi/mill#3748 downstream

Not sure if there's a better way to fix this? Just opening this to spark discussion

The original bugs are fine, but the solution seems incorrect, and is both overly conservative (invalidating everything based on whitespace?) and not conservative enough (doesn't handle cycles with length > 2?).

I don't know if there's a better answer here, but "invalidate everything in a cycle every time you touch a single file" is problematic, so much so that occasional crashes due to undercompilation seems preferable to universal slowness due to over-compilation. After all, the cost of a compiler crash is just the time spent on a full-clean-recompilation, so the frequency/pain of that needs to be weighed against the time spent re-compiling stuff unnecessarily the other 99% of the time. Maybe we can make it opt-in/out so people can choose the tradeoff that works for themselves?

Now I hate dependency cycles as much as anybody (hence https://github.com/com-lihaoyi/acyclic), but many real-world codebases I have to deal with are not nearly as strict, so I'd like Zinc to remain fast when working with them

@Friendseeker
Copy link
Member

[error] 	source-dependencies/abstract-class-to-trait
[error] 	source-dependencies/false-error
[error] 	source-dependencies/nested-type-params
[error] 	source-dependencies/no-type-annotation

@lihaoyi Would you like to rename

/zinc/src/sbt-test/source-dependencies/abstract-class-to-trait/test ->/zinc/src/sbt-test/source-dependencies/abstract-class-to-trait/pending

(and vice versa for the other 3 tests)?

Since's Mill's next release is imminent and Mill build codes heavily feature mutual reference, this is a blocking issue for Mill and I am thinking if we should make a new Zinc release ASAP.

While Mill downgrading Zinc is certainly an option, Zinc fixed a significant performance regression that affected mixed Scala / Java projects so downgrading it would cause a whole different set of slowness problem.

@Friendseeker Friendseeker self-requested a review October 17, 2024 00:52
@Friendseeker Friendseeker requested a review from eed3si9n October 17, 2024 00:53
Copy link
Member

@eed3si9n eed3si9n left a comment

Choose a reason for hiding this comment

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

LGTM. We can always try again on the fix now that we know about the general problem with circular deps.

@lihaoyi
Copy link
Contributor Author

lihaoyi commented Oct 17, 2024

Thanks @eed3si9n ! Could you help me cut a release once this is merged? So I can include it in Mill and close out that issue there

@eed3si9n
Copy link
Member

Yea. I've been holding off on making a release since we've added a new Java interface and it's good to batch changes. As far as I know we should be able to get something out soon.

@eed3si9n eed3si9n merged commit a290041 into sbt:1.10.x Oct 17, 2024
9 checks passed
@lihaoyi
Copy link
Contributor Author

lihaoyi commented Oct 18, 2024

Thanks @eed3si9n, if you can get it out in the next few days let me know and I'll include it in Mill 0.12.0

@eed3si9n
Copy link
Member

Yea. I've started the gradual release towards a 1.10.x patch release - sbt/sbt#7776. Hopefully we'll get a new Zinc out today or tomorrow.

@eed3si9n
Copy link
Member

SethTisue added a commit to SethTisue/scala that referenced this pull request Oct 21, 2024
as part of our sbt 1.10.3 upgrade, which pulls in
sbt/zinc#1462
which disables the same tests in the zinc repo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants