-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Added 'standalone' option when building tests for ilasm roundtrip #93037
Conversation
Tagging subscribers to this area: @hoyosjs Issue DetailsSet the environment variable
|
Can you please show a lab run with this ilasm flag running? |
- script: $(Build.SourcesDirectory)/src/tests/build$(scriptExt) $(logRootNameArg)Managed allTargets skipnative skipgeneratelayout skiptestwrappers $(buildConfig) $(archType) $(runtimeFlavorArgs) $(crossArg) $(priorityArg) $(testTreeFilterArg) ci /p:TargetOS=AnyOS | ||
displayName: Build managed test components | ||
- ${{ if in(parameters.testGroup, 'ilasm') }}: | ||
- script: $(Build.SourcesDirectory)/src/tests/build$(scriptExt) $(logRootNameArg)Managed allTargets skipnative skipgeneratelayout standalone skiptestwrappers $(buildConfig) $(archType) $(runtimeFlavorArgs) $(crossArg) $(priorityArg) $(testTreeFilterArg) ci /p:TargetOS=AnyOS |
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 think you should create an $(ilasm) so that you don't duplicate the entire script line. It will be easy for this to go stale.
I have some concerns about doing this. I think you may run into trouble with the "two pass" nature of building tests, but testing ilasm will determine if that is an actual issue. But also I'm not sure that you can just change the build arguments and still have coherent pipelines. For example, the artifact name is going to be the same. This means that normal and ilasm builds can't be in the same pipeline, which perhaps is already the case but would be an unfortunate dependency. Also, typically one has expectations about what is in a particular artifact based on the name. There may be additional issues with the overall pipelines so it would be good to get a review from someone more knowledgeable than me. |
https://dev.azure.com/dnceng-public/public/_build/results?buildId=427945&view=results should be that |
I don't see any changes compared to normal. The test artifacts zip has main-less dlls, and the test runs appear to be ildasm/ilasm-ing the test executors and then calling tests in dlls. |
@markples This is what I get when I compile a test as standalone and do |
I don't know what you mean about the test wrapper. I see no evidence that your change has changed any behavior in the lab. |
Meaning the test wrapper is dependent on Runtime_92590.dll and not Runtime_92590.asm.dll. |
.asm.dll is created during test execution in the .cmd/.sh files. The new wrappers depend on the .dlls for intra-proc tests, but the build system has no knowledge of the .asm.dll thing happening at test execution time. The new wrappers and the old infrastructutre depend on calling the .cmd/.sh files and have no knowledge of what is being executed within them. However, the problem here is deeper than that. I don't see entry points in the individual tests outside of ones that normally get them via RequiresProcessIsolation. There isn't anything for the roundtripping infrastructure to invoke even if it tried. |
I don't see how that matters. I still don't think the roundtripping logic is even being called. If you look at any of the logs, you'll see something like this:
For |
Well, it's now getting errors with |
Ok, I see that is only does it for RequiresProcessIsolation. It's interesting because if I only build a single test that doesn't have RequiresProcessIsolation, and I have BuildAsStandalone=true, I can run the roundtrip test on it. |
2 things going on here
|
@markples I think this is now running the roundtrip tests, though failing in Unix, the Windows versions passed: https://dev.azure.com/dnceng-public/public/_build/results?buildId=432469&view=logs&j=bc936583-455b-59a6-ba6a-b6595d929238&t=b93c0282-5af1-5880-fe4a-b9ec04a10b26 |
Ah, but now I see it only did it for RequiresProcessIsolation tests. Just checked Kusto and looked at the JitBlue tests and there are only a handful of them. Which was said before, this happens by default; means the standalone didn't do the right thing. |
…com/TIHan/runtime into ilasm-roundtrip-build-as-standalone
Closing in favor of #93368 |
Set the environment variable
BuildAsStandalone=true
when we are building tests for ilasm roundtripping.Current pipeline run: https://dev.azure.com/dnceng-public/public/_build/results?buildId=427945&view=results