-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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 JIT rolling build issue with non-JIT changes #64480
Fix JIT rolling build issue with non-JIT changes #64480
Conversation
When uploading a JIT rolling build, use the git hash of the most recent JIT change, not the git hash that was actually built. This handles the case where a JIT change kicked off an AzDO pipeline, but the pipeline didn't start until after one or more additional changes were merged. The AzDO pipelines appear to fetch with `-depth=20`, which should provide enough history for almost any case. Fixes dotnet#64392
Tagging subscribers to this area: @JulieLeeMSFT Issue DetailsWhen uploading a JIT rolling build, use the git hash of the most recent The AzDO pipelines appear to fetch with Fixes #64392
|
I tested the "walk back" logic locally with various git hashes. I don't want to spend the effort testing it in the CI, since that would require very complicated setup including merging JIT and non-JIT changes almost concurrently. We'll just see how it goes when it's live. |
@dotnet/jit-contrib |
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.
Few thoughts.
# Enumerate the last change, starting with the jit_git_hash, that included JIT changes. | ||
command = [ "git", "log", "--pretty=format:%H", jit_git_hash, "-1", "--", "src/coreclr/jit/*" ] | ||
print("Invoking: {}".format(" ".join(command))) | ||
proc = subprocess.Popen(command, stdout=subprocess.PIPE) |
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 not use run_command
from jitutil.py`?
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.
I copied/modified this code from elsewhere in jitrollingbuild.py. Looks like jitrollingbuild.py never uses run_command. In fact, it currently doesn't use jitutil at all. That might be a worthwhile (separate) cleanup at some point.
@@ -684,6 +718,11 @@ def setup_spmi_location_arg(spmi_location): | |||
lambda unused: True, | |||
"Unable to set git_hash") | |||
|
|||
coreclr_args.verify(args, | |||
"use_latest_jit_change", |
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 is this even an option? Why not do this way always? When will we ever pass use_latest_jit_change=False
?
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.
When I'm testing "upload" locally, I often give it non-actual hashes, so I want that to still 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.
LGTM
When uploading a JIT rolling build, use the git hash of the most recent
JIT change, not the git hash that was actually built. This handles the case
where a JIT change kicked off an AzDO pipeline, but the pipeline didn't start
until after one or more additional changes were merged.
The AzDO pipelines appear to fetch with
-depth=20
, which should provideenough history for almost any case.
Fixes #64392