aws-extensions-for-dotnet-cli
aws-extensions-for-dotnet-cli copied to clipboard
aws
Use MSBuild to Determine Target Framework for a Project
There are more ways to specify the target framework for a project than
setting it directly in the TargetFramework element in a
project. There are other unambiguous scenarios which this tool should
be able to support:
TargetFrameworkin the project fileTargetFrameworksin the project file, with one framework specified- Either 1 or 2 explicitly imported via
<Project Import=""/> - Either 1 or 2 implicitly imported via a means like "Directory.Build.props"
- Probably other things???
We ought to be able to support anything that MSBuild does by deferring the determination of target framework to MSBuild. For cases in which it returns multiple results (or no results, I suppose, though I don't know how that would build in the first place), the existing fallback mechanism is triggered.
Issue #, if available: N/A
Description of changes:
Defers determination of target framework to MSBuild. Removes XML parsing.
Combines "UtilitesTests" and "UtilitiesTests", as the former looked like a mistake. Happy to revert, if not.
Notes:
- This method of exfiltrating information from MSBuild is also used by other .NET tools.
- ~This PR is much smaller than it looks; a lot of the created files fill out the projects used in unit testing. (They're copied from
TestFunction.) I could customize these more if you tell me that there are elements inTestFunctionwhich apply only toTestFunction.~ I did it anyway.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Fwiw I think that Buildalyzer is a heavy handed approach for this problem, and I say that as a user (and big fan!) of Buildalyzer. A lighter touch approach would be to locate the MSBuild from the .NET SDK and use that to do a project evaluation, I'll see if I can cook up an example.
I'll see if I can cook up an example.
Great! I'm looking forward to seeing how that looks.
I have some code here that I haven't been able to test (I haven't even run it once!), but hopefully get's the idea across: https://github.com/chrisoverzero/aws-extensions-for-dotnet-cli/pull/1
It uses MSBuildLocator to find the latest SDK version of MSBuild, then when the MSBuild code is jitted .NET will try to load the assembly, it is important that we register the sdk location ahead of that because we need to intercept the assembly load event to get it to find the SDK located version. After that it's a case of evaluating the project.
In the project file, ExcludeAssets="Runtime" is used to prevent MSBuild from being outputted to the bin directory, but the reference is there for compile time.
Yup, I'm running with this. I'll have it cleaned up and working for commit in… probably a couple of hours.
ETA: Or one hour, whenever.
Thanks for the PR. Out of curiosity is this a blocking issue for you or helping us have a better complete solution?
There are a few tests failing because it looks like the MSBuildInitializer has not been initialized in that code path. To avoid those types of issues should we make the initialize of MSBuildInitializer happen lazily when LookupTargetFrameworkFromProjectFile is invoked?
Annoying issue we will have with this is this code is also consumed directly into the AWS Toolkit for Visual Studio 2017 and 2019. With 2017 we will need to down version of MSBuild.Build to version 15.* which targets .NET Framework 4.6 which is the minimum for VS 2017.
Level setting expectations this will take additional work to get working in VS because of the new dependencies and getting them packaged work. So I'll need to get some time to do that work before I can approve this PR.
There are a few tests failing because […]
Hm, can you let me know which ones? I can't recreate. Though I don't have a scratch AWS account in which to run Amazon.Lambda.Tools.Integ.Tests and Amazon.Lambda.Tools.Test. Do tests in there maybe call into the CLI libraries without invoking Main?
To avoid those types of issues […]
I can test that, sure. Unless @slang25 knows off-the-top that it won't work. There's some kind of ordering requirement.
Level setting expectations […]
Sure thing. I'll of course get this into a state where it's passing all tests as a foundation for that work.
[…] is this a blocking issue for you or helping us have a better complete solution?
Certainly not blocking because we can sprinkle aws-lambda-tools-defaults.json files around. But in some of my company's projects, we refactored common settings into Directory.Build.props, and we were surprised when it didn't work.
With 2017 we will need to down version of MSBuild.Build to version 15.* which targets .NET Framework 4.6 which is the minimum for VS 2017.
I have tests now passing with 15.1.548, which MS calls out in documentation as the version for VS2017. I'll wait to commit until I know what's going on with tests failing for others.
Hm, can you let me know which ones? I can't recreate.
Yes, I can, it's CreatePackageTests.
There's something I don't understand happening because I can get CreatePackageTests to pass when restricting to the 2.1 or 3.1 SDK with global.json. But under 5.0, it fails to find "Microsoft.DotNet.PlatformAbstractions". I see now that "Microsoft.DotNet.PlatformAbstractions" has been removed from the 5.0 SDK. I have no idea what to do about this.
I'll look at that lazy initialization, it's a bit tricky in that it must be run before the method that contains the MSBuild code is jitted, but it should be doable. If this code is being used outside of a .NET Core tool, we might need to make it a bit more forgiving and have the old behavior kick in.
"If only we had a way to run dotnet commands from here in the tool," he thought, failing to realize.
The target framework determination now exfiltrates MSBuild properties (TargetFramework and/or TargetFrameworks) by means of a custom target. This is a technique used by other .NET tools, and flips the previous strategy inside-out – don't bring MSBuild into the process, but make it export data which we can use. This now introduces no new package dependencies.
If you have ideas on how to make it any cleaner, I'm all for it.
It may be worth looking at these for approaches to get the current dotnet the tool is running under which would remove the assumption that dotnet is on the path (which is a pretty reasonable assumption tbf).
https://til.cazzulino.com/dotnet/how-to-locate-dotnet
Took a quick look and I like this approach a lot better not adding new dependencies to the tooling which always cause us a headache. Slammed right now but will try and take a deeper look soon.
Hi, @normj. I imagine probably that you’re still slammed, what with .NET 6 coming. But do you have an idea as to when this can be looked at (without revealing the timing and/or direction of future AWS features)?
Still no urgency, of course. Only circling back on a nice-to-have. I can rebase, too, if it would help.
It's too hard to rebase against a year and a half of changes. I'm declaring bankruptcy.
I will restart the work against the latest master branch. There are now a lot of properties which are read from the .*proj via XPath, and I think they all could instead be read accurately using this same technique.