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 dependency on Microsoft.Build.Utilities #2129

Closed
wants to merge 8 commits into from

Conversation

dazinator
Copy link
Member

Remove unnecessary dependencies on Microsoft.Build.* packages in an effort to help alleviate GitVersionTask errors when running from with VS, some of which described here: #2125

@dazinator
Copy link
Member Author

@arturcic - this may sound stupid, but how do I find the nuget package for this PR so I can give it a try? Do PR nuget packages get published anywhere?

@arturcic
Copy link
Member

I think you can use appveyor's feed

Co-Authored-By: Joseph Petersen <josephp90@gmail.com>
- It was broken, and hard to debug due to static initialisers that can't set a break point in.
- .MsBuild project -> netstandard. This was attempted before but due to cross compilation it would have caused issues, has to move platform specific code into GitVersionTask.
@dazinator
Copy link
Member Author

dazinator commented Feb 23, 2020

@arturcic
I managed to fix some issues, but I have hit a new issue in my PR, wondering if you have any knowledge on it.
When I build the GitVersionTask project, in the output folder for netstandard2.0, its missing the "runtimes" subfolder where the LibGit2sharp package is meant to place its native dependencies. Subsequently when I copy this output folder to give GitVersionTask a test (from dotnet build) as these binaries are missing from netstandard build output, where the task is loaded from, LibGit2sharp fails to load. Is there something special I need to do for .netstandard builds to force libgit2sharp to output these binaries? Or do we perhaps only include them in the nuget package as a seperate build step at nuget pack time?

@arturcic
Copy link
Member

https://github.com/GitTools/GitVersion/blob/master/src/GitVersionTask/nuget-files.props, This is the file that specifies how the nuget file is generated. It is created when calling dotnet pack from cake

@arturcic
Copy link
Member

I think we can release the new version without this issue, and work on this on the next version. @dazinator thoughts?

@dazinator
Copy link
Member Author

dazinator commented Feb 24, 2020

@arturcic I am pretty sure the new version is broken for atleast some msbuild scenarios. If there is no rush on getting the next version out it might be better to add some checks that exercise msbuild as per #2130 first, and then these will help catch regressions in future also.. what do you think?

@arturcic
Copy link
Member

The net472 is broken, what about the netcore?

@arturcic
Copy link
Member

We are testing in containers the netcore version of the task and it seems to be working

@dazinator
Copy link
Member Author

dazinator commented Feb 24, 2020

Yeah dotnet build works and I saw the checks for that. It was just msbuild I was hitting issues with - which is equivalent to building a project from VS.

@arturcic
Copy link
Member

msbuild with .netcore or with net472?

@dazinator
Copy link
Member Author

Net472

@arturcic
Copy link
Member

Ok I'll think about

@dazinator
Copy link
Member Author

I saw some existing logic for example when resolving dependencies from the 472 folder, it only resolves System.* - so where we have added a dependency to Micorsoft.Extensions.DependencyInjection that was causing an exception for me. I should have pushed up a specific fix for that but instead I went on a bit or anrefacoting exercise in my PR as a lot of the initialisation logic was in a static constructor where I couldnt hit any break points. All this is me testing on my local environment though, so I am hesitant to say anything I have discovered is absolutely conclusive until we also catch it with a check / test on a build server.

@arturcic
Copy link
Member

So the first think we should do is to have tests for msbuild in containers similar to the dotnet build to catch the issue, then to fix the issue, right?

@dazinator
Copy link
Member Author

dazinator commented Feb 24, 2020

Yeah that would be really good. Maybe starting with whatever the current stable msbuild version is. Then we can always add other versions tied to particular VS versions we want to support after that. A stretch goal would be to also run a check where it dynamically installs the latest preview version of msbuild corresponding to the current VS preview release channel, and tests using that (that would allow us to discover if GitVersionTask will not work with the latest release of msbuild rolled out to VS preview users - and potentially do something about it like update documentation or create a fix etc). Probably a bit optimistic on my part there.

@dazinator
Copy link
Member Author

dazinator commented Feb 24, 2020

Note to self:
On .NET Core, the reason why when any of the MSBuild task's are Execute()'d we essentially forward the call to a static Type that can run the execution logic for that task, is because..

When MSBuild loads the Task on .NET Core, it loads the type into its own AssemblyLoadContext, over which we don't have any control in terms of resolving dependencies as they are needed.
Therefore we create our own AssemblyLoadContext and then any assembly that we load with that, we can handle how its dependencies are then loaded - from the same location as the task assembly - or native dependencies from the "runtimes" folder i.e for libgit2sharp.

Because of this, we can't place any logic that would cause a dependency to load in the typical Task.Execute() method that MSBuild calls, because that will fail when MSBuild is unable to resolve those dependencies from its own AssemblyLoadContext.
Likewise, and this the important bit - we can't load the same Task type from our own AssemblyLoadContext and then attempt to use that, because if you try to use the same Type from two different AssemblyLoadContext's then you get weird exceptions along the lines of "Cannot cast Type A to Type B" even though the types appear to be identical.

So the approach I refactored to in this PR doesn't currently work for .NET Core because I attempt to load the same task Type from our own AssemblyLoadContext, then create an instance of it, and then Invoke a method on it: OnProxyExecute() - the idea being the task can then still encapsulate its own logic on the OnProxyExecute() method instead of OnExecute().

I had forgot about this issue of trying to use the same type from two different LoadContexts, and that is why tests in my PR currently fail after my refactoring. I need to change it back to along the lines of leveraging the pattern we had prior to my PR:

  1. MSBuild loads the Task and executes it.
  2. We create our custom AssemblyLoadContext on .NET Core as normal, but rather than resolve the same Task type and attempt to invoke some method on it, we instead load and invoke a type that only get's used within the new AssemblyloadContext -to run the task logic.

Knowing this I'll try to fix up the failing tests.

Note: interestingly, this issue doesn't exist under full .NET, as in that case we can handle how msbuild resolves assemblies, using AssemblyResolve events for the current app domain. Therefore when a task is executed, as long as our AssemblyResolve ebent handlers are in place, we can execute task logic as normal at that point. However I imagine we want the execution pipeline for tasks to look consistent irrespective of Platform.

@arturcic
Copy link
Member

Note to self:
On .NET Core, the reason why when any of the MSBuild task's are Execute()'d we essentially forward the call to a static Type that can run the execution logic for that task, is because..

When MSBuild loads the Task on .NET Core, it loads the type into its own AssemblyLoadContext, over which we don't have any control in terms of resolving dependencies as they are needed.
Therefore we create our own AssemblyLoadContext and anything assembly we load with that, we can handle loading its dependencies from the same location as the task assembly - or native dependencies from the "runtimes" folder i.e for libgit2sharp.

Because of this, we can't just execute the Task as it is, when MSBuild has loaded it from its own AssemblyLoadContext.
Likewise, and this the important bit - we can't load the same Task type from our own AssemblyLoadContext and then attempt to use it, because if you try to use the same Type from two different AssemblyLoadContext's then you get weird exceptions along the lines of "Cannot cast Type A to Type B" even though the types appear to be identical.

I had forgot about this issue, and that is why tests in my PR currently fail after my refactoring. I need to change it back to using this pattern:

  1. MSBuild loads the Task and executes it as normal.
  2. We create our custom AssemblyLoadContext on .NET Core as normal, but rather than resolve the same Task type and attempt to invoke it, we need to load a type that is only used in this new AssemblyloadContext - we had this before my current refactoring in the form of a static class, with different static methods that can be called to execute different tasks.

Knowing this I'll try to fix up the failing tests.

Note: interestingly, it doesn't seem necessary to load any proxy type under full .NET, as in that case we can handle how msbuild resolves assemblies, using AssemblyResolve events for the current app domain. However I imagine we want the execution pipeline for tasks to look consistent so the core logic is always in the same place regardless of platform.

I could not explain better the current implementation.

@dazinator
Copy link
Member Author

@arturcic I think I'll add a README or something to the GitVersionTask project - I knew this about 8 months ago but had forgotten :-(

@arturcic
Copy link
Member

@arturcic I think I'll add a README or something to the GitVersionTask project - I knew this about 8 months ago but had forgotten :-(

works for me

@dazinator
Copy link
Member Author

Abandoning this, in favour of a new one.

@dazinator dazinator closed this Mar 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants