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

Delete AssemblySpec::m_wszCodeBase #68186

Merged
merged 6 commits into from
Apr 19, 2022
Merged

Delete AssemblySpec::m_wszCodeBase #68186

merged 6 commits into from
Apr 19, 2022

Conversation

jkotas
Copy link
Member

@jkotas jkotas commented Apr 18, 2022

This was only used for assembly loading using hosting APIs that can be short-circuited to load directly without going through AssemblySpec

This was only used for assembly loading using hosting APIs that can be short-circuited to load directly without going through AssemblySpec
@ghost
Copy link

ghost commented Apr 18, 2022

Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov
See info in area-owners.md if you want to be subscribed.

Issue Details

This was only used for assembly loading using hosting APIs that can be short-circuited to load directly without going through AssemblySpec

Author: jkotas
Assignees: jkotas
Labels:

area-AssemblyLoader-coreclr

Milestone: -

@jkotas jkotas requested review from VSadov and elinor-fung April 18, 2022 22:54
Copy link
Member

@elinor-fung elinor-fung left a comment

Choose a reason for hiding this comment

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

Yay!

if (assemblyPath.MatchCaseInsensitive(pos, s_httpURLPrefix))
{
// HTTP downloads are unsupported
hr = FUSION_E_CODE_DOWNLOAD_DISABLED;
Copy link
Member

Choose a reason for hiding this comment

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

I think we can remove the FUSION_E_CODE_DOWNLOAD_DISABLED error code and message too.

Copy link
Member Author

Choose a reason for hiding this comment

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

This and other similar no-longer-used HRESULTs are coupled with the managed exceptions today. It would be nice to clean them up, but I am not going to do in this PR.

src/coreclr/binder/assemblybindercommon.cpp Show resolved Hide resolved
@vitek-karas
Copy link
Member

I didn't look into the details yet - but does this mean we can't really fix #59669 even if we wanted to?

@elinor-fung
Copy link
Member

I think the only thing that a fix for #59669 would need that this PR deletes is the field so that code base can be propagated somehow. The vast majority of this cleanup is the convoluted handling we have for when is/isn't codebase set, which we shouldn't need just to be able to propagate the value to extension points. I actually think this PR would make any fix for that issue more straight-forward, since it becomes an issue of propagating a property rather than propagating a property and trying to fit it into some existing mechanism that has different logic/behaviour for binding.

@jkotas
Copy link
Member Author

jkotas commented Apr 19, 2022

The codebase is missing from multiple layers between Assembly.Load and the managed resolver already (e.g. BindUsingHostAssemblyResolver takes BINDER_SPACE::AssemblyName that does not have codebase). If we wanted to fix #59669, I think we should pass the original AssemblyName instance through the layers and to the managed resolver, and avoid the back and forth conversions.

1 similar comment
@jkotas
Copy link
Member Author

jkotas commented Apr 19, 2022

The codebase is missing from multiple layers between Assembly.Load and the managed resolver already (e.g. BindUsingHostAssemblyResolver takes BINDER_SPACE::AssemblyName that does not have codebase). If we wanted to fix #59669, I think we should pass the original AssemblyName instance through the layers and to the managed resolver, and avoid the back and forth conversions.

Copy link
Member

@VSadov VSadov left a comment

Choose a reason for hiding this comment

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

Nice cleanup!

@jkotas jkotas merged commit eb9dd67 into dotnet:main Apr 19, 2022
@jkotas jkotas deleted the codebase branch April 21, 2022 03:45
directhex pushed a commit to directhex/runtime that referenced this pull request Apr 21, 2022
* Delete AssemblySpec::m_wszCodeBase

This was only used for assembly loading using hosting APIs that can be short-circuited to load directly without going through AssemblySpec

* Delete unnecessary ownedFlags argument
@ghost ghost locked as resolved and limited conversation to collaborators May 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants