Skip to content

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

Merged
lambdageek merged 4 commits intodotnet:mainfrom
lambdageek:fix-gh-54835
Jul 7, 2021
Merged

[mono] Don't call Assembly.CodeBase directly in RuntimeAssembly.GetName#54895
lambdageek merged 4 commits intodotnet:mainfrom
lambdageek:fix-gh-54835

Conversation

@lambdageek
Copy link
Member

@lambdageek lambdageek commented Jun 29, 2021

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
@ghost
Copy link

ghost commented Jun 29, 2021

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
Copy link
Contributor

@marek-safar marek-safar Jun 29, 2021

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

@lambdageek lambdageek Jun 30, 2021

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

@marek-safar marek-safar Jun 30, 2021

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

@lambdageek lambdageek Jun 30, 2021

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

lambdageek commented Jul 2, 2021

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

marek-safar commented Jul 2, 2021

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