-
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
Disable AOT build of GitHub_27678 under Mono #84380
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak Issue DetailsThis is a known failure (in issues.targets), but merged test groups cause the AOT failure to fail the entire build of Regression_3. MonoAotIncompatible (and RequiresProcessIsolation) separate them and allow the test to be failed (and skipped) separately.
|
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
There is a fix here: |
|
||
<!-- * Assertion at | ||
/__w/1/s/src/mono/mono/mini/memory-access.c:120, condition `size < MAX_INLINE_COPY_SIZE' not met --> | ||
<MonoAotIncompatible>true</MonoAotIncompatible> |
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.
Why do we need to mark the test as MonoAOT-incompatible? I would expect that just by using the RequiresProcessIsolation you should fix the build error as the test script will never be run (having been filtered out by the issues.targets-based exclusion list for the merged wrapper); adding the MonoAotIncompatible
property seems to indicate that this test can basically never run under MonoAOT by design and I don't see why it should apply here - once the MonoAOT bug has been fixed and the issues.targets exclusion removed, the test should start running again; with this annotation it will remain buried for eternity until someone happens to notice it and / or manually audit such annotations.
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.
(side note - appears to be moot for this PR given the fix)
That's a good point. I've so used to adding RequiresProcessIsolation for existing tags like GCStressIncompatible that I followed that here - perhaps considering the MonoAOTIncompatible to be a bit of documentation. But you're right and that documentation can simply be a comment. It would probably make sense to add a comment in issues.targets to remove the RPI as well. I'll do that for future issues (or here if the PR falls through for some reason). Thanks!
Closing - will modify and reopen if needed |
This is a known failure (in issues.targets), but merged test groups cause the AOT failure to fail the entire build of Regression_3. MonoAotIncompatible (and RequiresProcessIsolation) separate them and allow the test to be failed (and skipped) separately.