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.

Sign up for GitHub

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

Improve superpmi-asmdiffs AzDO pipeline robustness #61819

Merged
merged 2 commits into from
Nov 19, 2021
Merged

Improve superpmi-asmdiffs AzDO pipeline robustness #61819

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
46f0aed
Improve superpmi-asmdiffs AzDO pipeline robustness
BruceForstall Nov 18, 2021
8b6cba3
Remove unneeded `success` variable from jitrollingbuild.py
BruceForstall Nov 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/coreclr/scripts/jitrollingbuild.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -750,7 +750,6 @@ def main(args):
return 1

coreclr_args = setup_args(args)
success = True

if coreclr_args.mode == "upload":
upload_command(coreclr_args)
Expand All @@ -764,7 +763,8 @@ def main(args):
else:
raise NotImplementedError(coreclr_args.mode)

return 0 if success else 1
# Note that if there is any failure, an exception is raised and the process exit code is then `1`
return 0

################################################################################
# __main__
Expand Down
21 changes: 18 additions & 3 deletions src/coreclr/scripts/superpmi_asmdiffs_setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,9 @@ def main(main_args):

Args:
main_args ([type]): Arguments to the script

Returns:
0 on success, otherwise a failure code
"""

# Set up logging.
Expand Down Expand Up @@ -152,6 +155,7 @@ def main(main_args):
git_exe_tool = os.path.join(git_directory, "cmd", "git.exe")
if not os.path.isfile(git_exe_tool):
print('Error: `git` not found at {}'.format(git_exe_tool))
return 1

######## Get SuperPMI python scripts

Expand All @@ -167,18 +171,22 @@ def main(main_args):
os.makedirs(base_jit_directory)

print("Fetching history of `main` branch so we can find the baseline JIT")
run_command(["git", "fetch", "origin", "main"], source_directory, _exit_on_fail=True)
run_command(["git", "fetch", "--depth=500", "origin", "main"], source_directory, _exit_on_fail=True)
Copy link
Member

Choose a reason for hiding this comment

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

why this change?

Copy link
Member Author

@BruceForstall BruceForstall Nov 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This fixes the problem we were seeing not finding a baseline JIT, e.g., https://dev.azure.com/dnceng/public/_build/results?buildId=1475182&view=logs&j=011e1ec8-6569-5e69-4f06-baf193d1351e&t=bf6cf4cf-6432-59cf-d384-6b3bcf32ede2.

The pipelines only fetch with depth=20. If there aren't any JIT changes in those 20, we wouldn't find a baseline JIT. So, fetching a lot more makes it more likely to find a baseline JIT change.

Copy link
Member Author

Choose a reason for hiding this comment

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

E.g., you can see the initial fetch here:

https://dev.azure.com/dnceng/public/_build/results?buildId=1475182&view=logs&j=011e1ec8-6569-5e69-4f06-baf193d1351e&t=01b23941-a81d-5e8f-0e4b-5c0fce1bb894

Copy link
Member

Choose a reason for hiding this comment

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

My impression was that git fetch origin main (without --depth argument) would fetch the entire history. Doesn't how it work?

Copy link
Member Author

@BruceForstall BruceForstall Nov 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Well, I'm no expert, but it appears the answer is "no". It appears that the first fetch possibly creates a "shallow" repo and you either need to expand that by fetching with a larger depth, or use fetch --unshallow to expand it. E.g., from the fetch docs:

--depth=<depth> Limit fetching to the specified number of commits from the tip of each remote branch history. If fetching to a shallow repository created by git clone with --depth=<depth> option (see git-clone(1)), deepen or shorten the history to the specified number of commits. Tags for the deepened commits are not fetched.


# Note: we only support downloading Windows versions of the JIT currently. To support downloading
# non-Windows JITs on a Windows machine, pass `-host_os <os>` to jitrollingbuild.py.
print("Running jitrollingbuild.py download to get baseline")
print("Running jitrollingbuild.py download to get baseline JIT")
jit_rolling_build_script = os.path.join(superpmi_scripts_directory, "jitrollingbuild.py")
_, _, return_code = run_command([
python_path,
os.path.join(superpmi_scripts_directory, "jitrollingbuild.py"),
jit_rolling_build_script,
"download",
"-arch", arch,
"-target_dir", base_jit_directory],
source_directory)
if return_code != 0:
Copy link
Member

Choose a reason for hiding this comment

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

You need to make sure that you return the actual error_code from jitrollingbuild.py file. Currently it always return success.

Copy link
Member Author

Choose a reason for hiding this comment

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

It turns out that doesn't matter: jitrollingbuild.py error model currently is to throw an exception, and that leads to a process error code of 1. I started changing jitrollingbuild.py, then realized it wasn't necessary.

Copy link
Member

Choose a reason for hiding this comment

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

Got it. Can you remove the unused success = True from the jitrollingbuild.py then?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

print('{} failed with {}'.format(jit_rolling_build_script, return_code))
return return_code

######## Get diff JIT

Expand Down Expand Up @@ -238,6 +246,11 @@ def main(main_args):
# Details: https://bugs.python.org/issue26660
print('Ignoring PermissionError: {0}'.format(pe_error))

jit_analyze_tool = os.path.join(jit_analyze_build_directory, "jit-analyze.exe")
if not os.path.isfile(jit_analyze_tool):
print('Error: {} not found'.format(jit_analyze_tool))
return 1

######## Set pipeline variables

helix_source_prefix = "official"
Expand All @@ -249,6 +262,8 @@ def main(main_args):
set_pipeline_variable("Creator", creator)
set_pipeline_variable("HelixSourcePrefix", helix_source_prefix)

return 0


if __name__ == "__main__":
args = parser.parse_args()
Expand Down