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

Jump to bottom

[mono] Don't call Assembly.CodeBase directly in RuntimeAssembly.GetName #54895

Merged
merged 4 commits into from
Jul 7, 2021
Merged

[mono] Don't call Assembly.CodeBase directly in RuntimeAssembly.GetName #54895

merged 4 commits into from
Jul 7, 2021

Conversation

lambdageek
Copy link
Member

It's marked as not available in single file apps.
Call the underlying get_code_base icall.

Fixes #54835

@lambdageek lambdageek requested a review from marek-safar as a code owner June 29, 2021 13:20
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@lambdageek lambdageek mentioned this pull request Jun 29, 2021
Closed
@lambdageek lambdageek added the area-VM-reflection-mono Reflection issues specific to MonoVM label Jun 29, 2021
@ghost
Copy link

ghost commented Jun 29, 2021

Tagging subscribers to this area:
See info in area-owners.md if you want to be subscribed.

Issue Details

It's marked as not available in single file apps.
Call the underlying get_code_base icall.

Fixes #54835

Author: lambdageek
Assignees: -
Labels:

area-System.Reflection-mono
Milestone: -

marek-safar
marek-safar reviewed Jun 29, 2021
View reviewed changes
src/mono/System.Private.CoreLib/src/System/Reflection/RuntimeAssembly.cs Outdated
@@ -252,7 +252,7 @@ private static void AddPublicNestedTypes(Type type, List<Type> types, List<Excep

public override AssemblyName GetName(bool copiedName)
{
return AssemblyName.Create(_mono_assembly, CodeBase);
return AssemblyName.Create(_mono_assembly, get_code_base (this, false));
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this an actual fix? What does it return for single-file like targets?

Copy link
Member Author

Choose a reason for hiding this comment

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

In Mono we have two names associated with an assembly. It's "filename" which corresponds to Assembly.Location which is NULL for single-file targets like WebAssembly. And "name" which is derived from the name that the assembly was called in the bundle which is returned for Assembly.CodeBase.

So yea this is the actual fix - the issue is that the CodeBase public getter is tagged as not available in single-file. But get_code_base is not. And it returns a reasonable value. So we use it directly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah and I don't think that's correct. The value is exposed via https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System/Reflection/AssemblyName.cs#L63 which says that the values should be empty string (null value effectively)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. Switched to using MonoImage:filename which will be null for bundles

lambdageek added 3 commits July 1, 2021 14:37
@lambdageek
[mono] Don't call Assembly.CodeBase directly in RuntimeAssembly.GetName
69f252a 
It's marked as not available in single file apps.
Call the underlying get_code_base icall.

Fixes dotnet#54835
@lambdageek
[icall] Use MonoImage:filename for RuntimeAssembly.get_code_base
56e1e4c 
For bundled asssemblies in single file scenarios, RuntimeAssembly.CodeBase will
be null, matching CoreCLR.
@lambdageek
Fixup after rebase on origin/main
f6741c6
@lambdageek
Copy link
Member Author

Ok, so now wasm is failing a codebase test in src/libraries/System.Reflection/tests/AssemblyTests.cs because we now return null for the codebase here.

        [Fact]
        public void CodeBase()
        {
            Assert.NotEmpty(Helpers.ExecutingAssembly.CodeBase);
        }

How does this test pass on CoreCLR single file? or is it skipped somehow? @eerhardt

@eerhardt
Copy link
Member

eerhardt commented Jul 2, 2021

How does this test pass on CoreCLR single file?

I don't believe the test is executed on CoreCLR single file. Honestly, I don't think any of our library tests are executed under CoreCLR single file.

cc @agocke @tlakollo @vitek-karas

@tlakollo
Copy link
Contributor

tlakollo commented Jul 2, 2021

I don't believe the test is executed on CoreCLR single file. Honestly, I don't think any of our library tests are executed under CoreCLR single file.

I believe that is controlled by the following switch -> https://github.com/dotnet/runtime/blob/main/src/libraries/tests.proj#L329
We only tests single file in System.Collections.Tests.csproj and System.IO.IsolatedStorage.Tests.csproj but could be enabled in other projects

@marek-safar
Copy link
Contributor

How does this test pass on CoreCLR single file? or is it skipped somehow?

I believe there is a single-file like PlatformDetection property.

We only tests single file in System.Collections.Tests.csproj and System.IO.IsolatedStorage.Tests.csproj but could be enabled in other projects

I think System.Reflection would be a very useful addition to the list.

@lambdageek lambdageek merged commit fcedb50 into dotnet:main Jul 7, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Aug 6, 2021
@lambdageek lambdageek deleted the fix-gh-54835 branch March 19, 2022 16:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@marek-safar marek-safar marek-safar approved these changes

@eerhardt eerhardt eerhardt approved these changes

@thaystg thaystg Awaiting requested review from thaystg thaystg is a code owner

@vargaz vargaz Awaiting requested review from vargaz

Assignees
No one assigned
Labels
area-VM-reflection-mono Reflection issues specific to MonoVM
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Mono's RuntimeAssembly.GetName calls CodeBase
4 participants
@lambdageek @eerhardt @tlakollo @marek-safar