-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Create IL-RT folder to store roundtripped IL assemblies and sources, removed suffix '.asm' to roundtripped files, re-enabled rejit ilasm roundtripping #90110
Conversation
/azp run runtime-coreclr ilasm |
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
/azp run runtime-coreclr ilasm |
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
@dotnet/jit-contrib @jakobbotsch this is ready. |
I kicked off a runtime-coreclr ilasm run from AzDO manually: https://dev.azure.com/dnceng-public/public/_build/results?buildId=366414&view=results |
I am concerned that trying to use a subdirectory is going to introduce a different set of problems when the test has dependencies on the folder/file structure. Perhaps some of that can be mitigated (as I think will be the case here) by executing the test with the test "root" as the working directory rather than the new subdirectory (for example, a test with an input text file in the current directory could still work). I'm not sure how a test finding library assemblies is going to work. Related - you probably need to address the ilasm test coverage issue with merged test groups before doing this. Maybe running the tests with a roundtripped wrapper but not the tests themselves is enough to test this, but I'm not sure. However, the library assemblies comment above is especially important because basically all test code is going to be in library assemblies, and you'll need this IL-RT setup to find the roundtripped assemblies rather than the originals. cc @jkoritzinsky who I believe suggested that ilasm roundtripping simply overwrite the originals. It trades simplicity of the ilasm testing scenario for the possibility of trashing the test directory if roundtripping goes badly, but that case is (1) rare and a developer can rebuild the test, and (2) not applicable in the lab where the test tree is discarded after a test run |
The way crossgen2 handles it is by moving the original assemblies into the |
IMO, any model where the existing tests are modified (overwritten, originals moved, etc.) is broken. That includes the current crossgen2 model. Running the tests in a particular mode should not alter the built artifacts. It should be trivial to get back to the "just built" situation. It should be trivial to rerun a test without worrying about it running on top of altered artifacts.
A test that goes badly and trashes the test directory may be rare, but it's an important scenario. |
There's a fundamental problem that r2r and ilasm are build steps that exist outside of the build [1] and the logic for them is general and exists outside of the test execution. It shows up in various ways - how do they find dependent assemblies being another (r2r appears to do *.dll, which I guess is somehow resilient to non-assemblies, or maybe those tests are all disabled?). A risk of not actually doing the test is another (ilasm is in this state today after merged groups). Test-specific dependencies on assembly filenames and other test assets are another. Copying the entire test directory elsewhere (and modifying it there) could be another attempt, but one problem with that would be any absolute paths (to test assets would be ok maybe? do these exist? I don't know). Or maybe a more robust "save the test directory, do stuff, restore it", though I don't think this strategy could be perfect against ctrl-cs and stuff like that. I supposed some kind of virtualized file system could it... I certainly don't like the idea of the test artifacts getting trashed, and it's easy for a "hey, if you're running r2r or ilasm, be aware that you might need to rebuild" rule being forgotten, leading to frustration. But I don't see any alternatives (or know who is going to do them). [1] I assume that we don't want running build.cmd or msbuild directly on a test project to (somehow) create multiple copies of a test and do those steps. Or have a separate build mode for them (such that the "anycpu" shared build isn't sufficient anymore, etc.) Maybe there's some hybrid. These are probably conceptually some correct fixes. |
I don't have a strong opinion here, and there seems to be some outstanding concerns in general regarding how we handle testing like this, including the existing CrossGen2 tests. For the purposes of moving this PR forward, what do you all recommend? Should we delay this to .NET 9? It's not a critical component for .NET 8. Another question, is the behavior better than before with this change? |
The ilasm run failed - looks like the test in the subdirectory can't find dependent assemblies. |
I see, thought the test runs propagated to this PR. Will look into it. |
Will need to copy DLLs to the IL-RT folder like crossgen2, "cp *.dll IL-CG2/" |
Since the test run was triggered manually, there was nothing to propagate any results back to here. ilasm.yml contains
so my guess is that the PR containing ilasm/ildasm changes is what blocked your attempt. You don't want to change those paths to include src/tests because I suspect it would automatically run the pipeline too much for others. You might be able to temporarily change that include or temporarily make a little change in ilasm/ildasm (assuming that the PR contents are examined and not main itself...) to get the github integration to work (of course make sure to revert before merging, which will retrigger the runtime job...) or continue to manually execute it. Note that your proposal is still different from r2r. r2r copies the originals out and modifies the originals (the point of contention), whereas I believe you are doing the modifications in the subdirectory. You could still hit issues with the subdirectory but could try it to see how it works. |
/azp run runtime-coreclr ilasm |
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
@markples I tried modifying ilasm.yml just by modifying whitespace, but it still wouldn't let me trigger it here. I manually triggered it though: https://dev.azure.com/dnceng-public/public/_build/results?buildId=367708&view=results |
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.
It looks like, as some expected, there are tests that depend on the directory structure.
Perhaps Jakob's suggestion of saving off the originals and modifying in place is the only solution, without some more drastic general solution (e.g., copying the entire directory structure to be a sibling directory?).
IMO, we should just push this work to .NET 9. Missing one test isn't terrible.
I'm more worried about:
Related - you probably need to address the ilasm test coverage issue with merged test groups before doing this.
@markples What test coverage are we missing? Is there a GitHub issue detailing it?
eng/pipelines/coreclr/ilasm.yml
Outdated
@@ -68,3 +68,4 @@ extends: | |||
jobParameters: | |||
testGroup: ilasm | |||
liveLibrariesBuildConfig: Release | |||
|
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.
Undo the (empty) change to this file
Tagging subscribers to this area: @JulieLeeMSFT |
@TIHan I guess there's a github-side configuration that I don't know about. Oh well. @BruceForstall It's listed in #71732 and @TIHan is aware. Short answer is most of it and the ilasm scripts only look at the entrypoint dll, and merged groups put the tests in other dlls. |
So we're not actually round-tripping the tests anymore? That sounds like a significant drop in test coverage. Presumably the "entrypoint dlls" are stylized so we aren't getting a variety of code patterns through the round-trip testing. |
That's correct. Only RequiresProcessIsolation would be preserved right now. |
@TIHan Can you mark this "Draft" until it's ready to review again? |
/azp run runtime-coreclr ilasm |
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
Only about 10 failures now. |
@BruceForstall this is ready - CI passed green - https://dev.azure.com/dnceng-public/public/_build/results?buildId=427905&view=results The original assemblies get copied to IL-RT |
// cc @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.
LGTM.
Please trigger the ilasm pipeline on your final changes and ensure the pipeline is clean before merging.
Resolves #71119
Resolves #71316