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.

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

ClrOnly condition is not working on mono #75548

Merged
merged 23 commits into from
Oct 22, 2024

Conversation

giritrivedi
Copy link
Contributor

This test case tests the Clr behaviour Only. ConditionalFact(typeof(ClrOnly) should skip the execution of the test case if it is executed on Mono. However, the condition doesn't hold good.
The fix takes care of skipping the execution tests on Mono which are meant for ClrOnly.

The functionality work both on little endian and bigendian architecture.
Hence removing the assert statements. These can be verified by running
CommandLine test suite.
Couple of visualBasic "EmitAndContinue" test cases fail with
"Operation is not supported on this platform" error.
Reason for these failure is that they are not supported on Mono.
Ensured that these test cases are skipped when run on mono.
This reverts commit 534273d.
Couple of visualBasic "EmitAndContinue" test cases fail with
"Operation is not supported on this platform" error.
Reason for these failure is that they are not supported on Mono.
Ensured that these test cases are skipped when run on mono.
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Oct 17, 2024
@dotnet-policy-service dotnet-policy-service bot added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Oct 17, 2024
@giritrivedi
Copy link
Contributor Author

r/azp

@giritrivedi
Copy link
Contributor Author

/azp run

Copy link

Commenter does not have sufficient privileges for PR 75548 in repo dotnet/roslyn

@giritrivedi
Copy link
Contributor Author

@jjonescz Can you please restart azure pipeline. This was failing due to dependency with previous commit.

@giritrivedi
Copy link
Contributor Author

\cc @uweigand

@jjonescz
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@giritrivedi giritrivedi marked this pull request as ready for review October 22, 2024 08:19
@giritrivedi giritrivedi requested a review from a team as a code owner October 22, 2024 08:19
@giritrivedi
Copy link
Contributor Author

@jjonescz Thanks for running the AZP. Would you mind reviewing the changes ?

@@ -373,7 +373,7 @@ static ImmutableArray<INamedTypeSymbol> getForwardedTypes(AssemblySymbol assembl
}
}

[ConditionalFact(typeof(ClrOnly), Reason = "Static execution is runtime defined and this tests Clr behavior only")]
[ConditionalFact(typeof(NotOnMonoCore), Reason = "Static execution is runtime defined and this tests Clr behavior only")]
Copy link
Member

@jjonescz jjonescz Oct 22, 2024

Choose a reason for hiding this comment

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

With NotOnMonoCore, the test would get executed on Mono Desktop, right? But it presumably wouldn't work there since it used to be marked ClrOnly. So should we use CoreClrOnly instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, with "CoreClrOnly", the test fails.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, right, is that because ExecutionConditionUtil.IsCoreClr is true when running on mono core?

Well, still, we want to skip this test on both MonoCore and MonoDesktop, don't we? Perhaps we should have a NotOnAnyMono attribute?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes you are right . IsCoreClr is just a negation of IsDesktop which just check "RuntimeUtilities.IsDesktopRuntime" which is true when running on mono core.
yes, We should have NotOnAnyMono attribute. Will make these changes .

@jaredpar jaredpar added the Platform - Mono CoreClr Bugs specific to mono coreclr label Oct 22, 2024
Co-authored-by: Jan Jones <jan.jones.cz@gmail.com>
@giritrivedi
Copy link
Contributor Author

Hi @jjonescz , Can this be merged ?

@jjonescz jjonescz merged commit 511a727 into dotnet:main Oct 22, 2024
24 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Oct 22, 2024
@jjonescz
Copy link
Member

Thanks @giritrivedi for the contribution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. Platform - Mono CoreClr Bugs specific to mono coreclr untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants