-
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
Improve superpmi.py scripting #42238
Improve superpmi.py scripting #42238
Conversation
@kunalspathak Here's my current set of changes. I need to do a little more testing, update the superpmi.md documentation, and write a summary of the changes for the commit message. |
src/coreclr/scripts/superpmi.py
Outdated
# If the user passed -temp_dir or --diff_with_code_only, we skip the SuperPMI replay process, | ||
# and rely on what we find from a previous run. | ||
if self.coreclr_args.temp_dir is not None or self.coreclr_args.diff_with_code_only: | ||
return_code = 1; |
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.
micro nit
return_code = 1; | |
return_code = 1 |
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.
That's what happens when you let a C programmer touch Python :)
src/coreclr/scripts/superpmi.py
Outdated
|
||
if not self.coreclr_args.diff_with_code_only: | ||
# Change the working directory to the core root we will call SuperPMI from. | ||
if self.coreclr_args.log_file != None: |
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.
micro nit:
if self.coreclr_args.log_file != None: | |
if self.coreclr_args.log_file is not None: |
src/coreclr/scripts/superpmi.py
Outdated
# 2. A copy of PMI.dll, as a fallback in case we need it but can't find it locally, | ||
# so we don't need to download dotnet/jitutils and build it ourselves. | ||
# (Note: if PMI is ever published as a package, we could just download that instead.) | ||
# 3. A copy of coredistools. If, when doing asm diffs, a copy of the coredistools |
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.
# 3. A copy of coredistools. If, when doing asm diffs, a copy of the coredistools | |
# 3. A copy of coredistools. If, when doing asm diffs, a copy of the coredistools |
1. Update Azure storage collections location to a new location where we have appropriate permissions to manage the data. 2. Update the Azure storage upload/download implementation to the current version of the Azure Python API. 3. Add JIT-EE interface GUID to path of Azure stored collections. When downloading collections, use the appropriate collection for your JIT-EE interface GUID. This is all done by adding a "-printJITEEVersion" option to the SuperPMI MCS tool. Thus, to determine the JIT-EE version, we assume that MCS is built with the same version as the JIT (which will be true in a normal build), and that MCS is available and able to be run -- this typically requires a Core_Root location be available. The user can specify the JIT-EE version explicitly with the new `-jit_ee_version` argument. 4. Simplify the Azure storage format: there is no longer a JSON mapping of name to MCH file. Instead, there is just a directory full of files. By default, all files are downloaded and used for replay/asmdiffs, by that can be filtered with the new `-filter` argument. 5. The `-mch_files` (previously `-mch_file`) argument used by `replay`, `asmdiffs`, and `upload`, now accepts a list of directories and files, and for each directory all MCH files included in that directory, recursively, are used. 6. Also upload MCT (TOC) files with the MCH files. 7. A `--force_download` argument is added to allow forcing re-download of the Azure collections to the local cache. 8. PMI.dll is also looked for on the PATH before downloading a cached version from Azure storage. 9. Some of the lesser-used arguments were renamed to simplify them. 10. Various bugs were fixed and code simplification/reorganization was done. E.g., some of the commonality between replay and asmdiffs was factored out. 11. More code documentation was added. 12. The superpmi.md documentation was re-written and simplified.
fab1f22
to
5cc1845
Compare
This is ready for review. @kunalspathak PTAL |
Btw, none of this is used in the product, and there are a lot of changes, so you can choose how detailed a review you think is warranted. |
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
where we have appropriate permissions to manage the data.
to the current version of the Azure Python API.
collections. When downloading collections, use the appropriate
collection for your JIT-EE interface GUID. This is all done
by adding a "-printJITEEVersion" option to the SuperPMI
MCS tool. Thus, to determine the JIT-EE version, we assume
that MCS is built with the same version as the JIT (which
will be true in a normal build), and that MCS is available
and able to be run -- this typically requires a Core_Root
location be available. The user can specify the JIT-EE
version explicitly with the new
-jit_ee_version
argument.a JSON mapping of name to MCH file. Instead, there is just
a directory full of files. By default, all files are downloaded
and used for replay/asmdiffs, by that can be filtered with
the new
-filter
argument.-mch_files
(previously-mch_file
) argument usedby
replay
,asmdiffs
, andupload
, now accepts a listof directories and files, and for each directory all MCH
files included in that directory, recursively, are used.
--force_download
argument is added to allow forcingre-download of the Azure collections to the local cache.
a cached version from Azure storage.
E.g., some of the commonality between replay and asmdiffs was
factored out.