Skip to content
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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Disable AOT build of GitHub_27678 under Mono #84380

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/tests/JIT/Regression/JitBlue/GitHub_27678/GitHub_27678.ilproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,12 @@
<PropertyGroup>
<DebugType>None</DebugType>
<Optimize>True</Optimize>

<!-- * Assertion at
/__w/1/s/src/mono/mono/mini/memory-access.c:120, condition `size < MAX_INLINE_COPY_SIZE' not met -->
<MonoAotIncompatible>true</MonoAotIncompatible>
Copy link
Member

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.

Copy link
Member Author

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!

<!-- Needed for MonoAotIncompatible -->
<RequiresProcessIsolation>true</RequiresProcessIsolation>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).il" />
Expand Down