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

Remove LoadFromAssemblyName for MSBuild assemblies in the CoreCLRAssemblyLoader #3900

Merged
merged 2 commits into from Nov 19, 2018

Conversation

@JoeRobich
Copy link
Contributor

commented Nov 1, 2018

From @vitek-karas,

Handler for the Resolve event should never call LoadFromAssemblyName (or Assembly.Load) – that will lead to recursive loops. Resolve event handlers should only load assemblies from sources (files, memory streams).

Fixes #3352

return assembly;
// Ensure we are attempting to load a matching version
// of the Microsoft.Build.* assembly.
assemblyName.Version = _currentAssemblyVersion;

This comment has been minimized.

Copy link
@rainersigwald

rainersigwald Nov 2, 2018

Contributor

Since we don't vary assembly version across builds/releases any more, I think we might have to do more here, like force the load to come from folder-next-to-current-assembly rather than doing the full lookup.

This comment has been minimized.

Copy link
@JoeRobich

JoeRobich Nov 2, 2018

Author Contributor

So I should extract the candidate path searching logic and call it from TryGetWellKnownAssembly with the executing assembly's location? something like:

        private Assembly TryGetWellKnownAssembly(AssemblyLoadContext context, AssemblyName assemblyName)
        {
            if (!_wellKnownAssemblyNames.Contains(assemblyName.Name))
            {
                return null;
            }

            // Ensure we are attempting to load a matching version
            // of the Microsoft.Build.* assembly.
            assemblyName.Version = _currentAssemblyVersion;

            var searchPaths = new[] { Assembly.GetExecutingAssembly().Location };
            return TryResolveAssemblyFromPaths(context, assemblyName, searchPaths);
        }

This comment has been minimized.

Copy link
@rainersigwald

rainersigwald Nov 2, 2018

Contributor

That sounds pretty good to me.

I remember some funkiness around getting path-to-current-assembly in .NET Core, but I bet it's fixed now, that was a pre-1.0 memory. It might be better to use BuildEnvironmentHelper.Instance.CurrentMSBuildExePath, but that might also accidentally trigger other loads nope, looks like that was all about targeting <netstandard1.3.

@tmeschter, can you dredge up any memories that might be relevant to this loader change?

This comment has been minimized.

Copy link
@tmeschter

tmeschter Nov 2, 2018

Member

I'm not sure how much of this code is necessary--I had to work around multiple bugs in the CoreCLR to get anything to work at the time, and I would assume they have been fixed now.

I can't remember now why I even needed special handling of the well-known assemblies. Since we're always using the default AssemblyLoadContext I would assume that all loads of Microsoft.Build.dll, etc, would resolve to the ones shipped with the product (regardless of version), so why would we ever need to handle those in the resolve assembly event handler? But probably I've just forgotten how AssemblyLoadContext works. :-)

The commit messages are your best change for understanding what I was thinking at the time.

@JoeRobich JoeRobich changed the title Remove special loading for MSBuild assemblies in the CoreCLRAssemblyLoader Remove LoadFromAssemblyName for MSBuild assemblies in the CoreCLRAssemblyLoader Nov 2, 2018
@rainersigwald

This comment has been minimized.

Copy link
Contributor

commented Nov 16, 2018

This is good to go, right? Anyone know of a reason to wait further?

@JoeRobich

This comment has been minimized.

Copy link
Contributor Author

commented Nov 16, 2018

@rainersigwald I don't have any additional changes planned. =)

@rainersigwald rainersigwald merged commit 3457a9f into microsoft:master Nov 19, 2018
4 checks passed
4 checks passed
RHEL7.2 Build for CoreCLR Build finished.
Details
WIP Ready for review
Details
license/cla All CLA requirements met.
Details
msbuild-pr #20181102.1 succeeded
Details
@rainersigwald

This comment has been minimized.

Copy link
Contributor

commented Nov 19, 2018

Thanks @JoeRobich!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.