Skip to content

Conversation

@BruceForstall
Copy link
Contributor

Add a superpmi.py collect -exclude option to avoid collecting
assemblies known to fail (and cause pop-up failures: see #47552).

Use -exclude to exclude R2RTest\Microsoft.Build.dll.

Depends on #47779

Fixes #47541

@ghost ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Feb 3, 2021
@BruceForstall
Copy link
Contributor Author

@dotnet/jit-contrib
cc @nattress

@BruceForstall
Copy link
Contributor Author

cc @dotnet/crossgen-contrib

Copy link
Contributor

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

Overall looks good (as long as superpmi collection pipeline is green) except a comment about moving --exclude from superpmi.py to superpmi-setup.py.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would recommend the right way to do this would be to not copy the assemblies that are problematic to the payload directory. superpmi-setup.py already has an ability to do that:

def partition_files(src_directory, dst_directory, max_size, exclude_directories=[], exclude_files=native_binaries_to_ignore):

Perhaps, just add a condition there that if the collection_name == 'crossgen2', exclude it.

Copy link
Contributor

Choose a reason for hiding this comment

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

With that, you won't need any changes in superpmi.py. The script will just act on the assemblies you provided.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I moved the exclusion to superpmi-setup.py. The superpmi.py -exclude code is not required now, but I'd like to keep it as it could be generally useful.

Copy link
Contributor

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@kunalspathak
Copy link
Contributor

I have kicked off a superpmi collection run based on this PR - https://dev.azure.com/dnceng/internal/_build/results?buildId=978167&view=results

@kunalspathak
Copy link
Contributor

I have kicked off a superpmi collection run based on this PR - https://dev.azure.com/dnceng/internal/_build/results?buildId=978167&view=results

Never mind. I just saw that it depends on #47779. I have cancelled the run.

Exclude R2RTest\Microsoft.Build.dll from automated collection.

Add a superpmi.py collect `-exclude` option to avoid collecting
assemblies known to fail (and cause pop-up failures: see dotnet#47552).
(This isn't used by the automated collection, but is generally useful.)

Fixes dotnet#47541
@BruceForstall BruceForstall force-pushed the AddCrossgen2Collection2 branch from f0a4662 to 086fe9f Compare February 4, 2021 00:59
@BruceForstall
Copy link
Contributor Author

Add .dotnet directory to path, so it can be found by superpmi.py when
setting up a crossgen2 collection.

Extra: if `-temp_dir` is specified to superpmi.py, normalize it to
an absolute path.
Fix Windows PATH setting XML separator escape character

Add some more logging output
1. exclude more native files from superpmi setup
2. write stdout and stderr files to collection log on failure
3. write superpmi collection error return code to log on failure
4. fix creation of log file directories when `-log_file` argument specified
5. stop logging too-verbose message about CORINFO_SIG_INST handle
array illegal values, since it's "by design" for crossgen2
This makes it easier to see the log output without needing to
find and download the logfile separately from the CI.
Allow copying .json files, which is required by crossgen2.

Don't filter anything in copy of .dotnet directory; I don't know
what kind of files are there, and don't want to filter any of them.
@BruceForstall
Copy link
Contributor Author

@BruceForstall
Copy link
Contributor Author

@kunalspathak I've made a bunch of additional changes here (you've probably noticed), in case you want to review them.

Copy link
Contributor

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

LGTM, with a minor suggestion.

# Need to accept files without any extension, which is how executable filesnames look.
acceptable_copy = lambda path: (os.path.basename(path).find(".") == -1) or any(path.endswith(extension) for extension in [".py", ".dll", ".so", ".json"])

print("is_windows {}".format(is_windows))
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete.

"clretwrc.dll",
"clrgc.dll",
"clrjit.dll",
"clrjit_unix_arm_arm.dll",
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for doing this. Wanted to do from long time.

@ghost ghost locked as resolved and limited conversation to collaborators Mar 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

superpmi: add crossgen2 collection to automated collections

2 participants