-
-
Notifications
You must be signed in to change notification settings - Fork 626
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
Enable incremental compile for zinc hermetic by adding scratch input digest for local ExecuteProcessRequest #8282
Conversation
This reverts commit 489220c.
Not reviewable yet, but cc @blorente : Yi is currently leaning toward a local-only sidechannel for stashing analysis. Potentially related to other local-only low-latency-JVM options. |
This is now ready for review. Thanks! Travis failed at unrelated tests. |
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.
Thanks a lot! Left some minor nits, but the overall design, as discussed, lgtm
tests/python/pants_test/backend/jvm/tasks/jvm_compile/java/test_zinc_compile_integration.py
Outdated
Show resolved
Hide resolved
src/python/pants/backend/jvm/tasks/jvm_compile/zinc/zinc_compile.py
Outdated
Show resolved
Hide resolved
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.
Thanks, generally looks great; a few things to fix up :)
src/python/pants/backend/jvm/tasks/jvm_compile/zinc/zinc_compile.py
Outdated
Show resolved
Hide resolved
src/python/pants/backend/jvm/tasks/jvm_compile/zinc/zinc_compile.py
Outdated
Show resolved
Hide resolved
return _classes_dir_snapshot | ||
|
||
analysis_snapshot = _get_analysis_snaoshot() | ||
classes_dir_snapshot = _get_classes_dir_snapshot() |
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.
Why do we need a classes dir? I thought Zinc could operate purely on jars? If this is necessary, can you add a comment to the code explaining why? This seems pretty weird...
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.
Indeed. zinc requires loose classfiles in the output directory in order for incremental compile to work.
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.
zinc was supposed to be headed in the direction of supporting operating on jars... but it didn't get there, unfortunately.
tests/python/pants_test/backend/jvm/tasks/jvm_compile/java/test_zinc_compile_integration.py
Outdated
Show resolved
Hide resolved
tests/python/pants_test/backend/jvm/tasks/jvm_compile/java/test_zinc_compile_integration.py
Outdated
Show resolved
Hide resolved
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.
Thanks Yi!
return _classes_dir_snapshot | ||
|
||
analysis_snapshot = _get_analysis_snaoshot() | ||
classes_dir_snapshot = _get_classes_dir_snapshot() |
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.
zinc was supposed to be headed in the direction of supporting operating on jars... but it didn't get there, unfortunately.
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.
Looks good, modulo outstanding comments - thanks!
Problem
Zinc hermetic does not work with incremental compile
Solution
Add local specific scratch dir for
ExecuteProcessRequest
withunsafe_local_only_files_because_we_favor_speed_over_correctness_for_this_rule: hashing::Digest
Similar to
input_files
, except it is ignored in the remote case.Reasoning being
classes/
into theinput_files
digest, it could cause severe performance issue for downloading a lot of loosing files in remoting.