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

Assembly.CodeBase obsoletion is incomplete #42857

Closed
Tracked by #44655 ...
agocke opened this issue Sep 29, 2020 · 10 comments · Fixed by #70534
Closed
Tracked by #44655 ...

Assembly.CodeBase obsoletion is incomplete #42857

agocke opened this issue Sep 29, 2020 · 10 comments · Fixed by #70534
Assignees
Milestone

Comments

@agocke
Copy link
Member

agocke commented Sep 29, 2020

Assembly.CodeBase and Assembly.EscapedCodeBase were marked obsolete in #31127

but this work was incomplete: AssemblyName.CodeBase and AssemblyName.EscapedCodeBase are not marked. In addition, usages of these obsolete APIs remain in the source code.

We need to carefully audit all the use-sites and either remove them or provide explicit justification for our continued use.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-AssemblyLoader-coreclr untriaged New issue has not been triaged by the area owner labels Sep 29, 2020
@ghost
Copy link

ghost commented Sep 29, 2020

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

@agocke agocke added this to the 6.0.0 milestone Sep 29, 2020
@agocke agocke added area-Infrastructure-libraries and removed area-AssemblyLoader-coreclr untriaged New issue has not been triaged by the area owner labels Sep 29, 2020
@ghost
Copy link

ghost commented Sep 29, 2020

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

@agocke
Copy link
Member Author

agocke commented Sep 29, 2020

cc @ericstj

@ericstj
Copy link
Member

ericstj commented Sep 29, 2020

Looks like @jkotas suggested pragma here: #31127 (comment)

Not sure about AssemblyName WRT original issue. Cc @jeffhandley @JoshSchreuder

@agocke
Copy link
Member Author

agocke commented Sep 29, 2020

At the stage of the release we were in, I think silencing the warnings was reasonable, but I don't think it's a long-term solution.

This issue just tracks finishing that long-term work.

@jeffhandley
Copy link
Member

I'd be supportive of adding AssemblyName.CodeBase and AssemblyName.EscapedCodeBase into the same obsoletion diagnostic id in 6.0 and also taking a sweep through our own usage to remove the #pragmas.

@jkotas
Copy link
Member

jkotas commented Sep 29, 2020

sweep through our own usage to remove the #pragmas

There is one pragma in the shipping code that #42306 is attempting to fix. The remaining pragmas are in method overrides where they are used by design, and in tests.

@agocke
Copy link
Member Author

agocke commented Sep 29, 2020

I think we should also take a look at code in other repos, like WPF and ASP.NET

@jeffhandley
Copy link
Member

@dotnet/area-system-reflection I spent some time looking into this, but I am not familiar enough with how AssemblyName is being used to determine the right path forward here.

I moved the issue to 7.0.0, but if time permits before .NET 6 RC1, this would be nice-to-have.

@buyaa-n
Copy link
Contributor

buyaa-n commented Jun 9, 2022

I don't see much real usage of CodeBase in source.dot.net, mostly setting the property from passed AssemblyName to the newly created/cloned AssemblyName. There is a few usage that try to load the assembly by setting CodeBase and seems it was the main functional usage:

private static Assembly LoadAssembly(string codeBase)
{
    AssemblyName assemblyName = new AssemblyName { CodeBase = assemblyPath };
    return Assembly.Load(assemblyName);
}

but this code would not work in .Net Core, therefore I think we should obsolete CodeBase/EscapedCodeBase and let developers avoid from issues like: AssemblyName.CodeBase is ignored in LoadFromAssemblyName while porting their app from .Net framework

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jun 10, 2022
Repository owner moved this from Future to Done in Triage POD for Reflection, META, etc. Jun 14, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jun 14, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jul 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

6 participants