-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Convert all projects to SDK-style #29831
Changes from 3 commits
91d7e62
be0421d
f60fa38
10b7151
3ecd281
ad505eb
be5ceb5
204a8db
1c91f96
3b36996
b09b0b5
67dacd6
0c7364e
53d5973
6d25667
e05508a
9f65193
ec546de
860888c
6ccae92
0343b37
1a4b8a2
9b7b8c0
e72643b
aa89672
b86cade
99e21e6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
<?xml version="1.0" encoding="utf-8"?> | ||
<Project ToolsVersion="14.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003"> | ||
<Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildProjectDirectory), dir.props))\dir.props" /> | ||
<PropertyGroup> | ||
<EnableDefaultItems>false</EnableDefaultItems> | ||
<NoWarn>CS3021;$(NoWarn)</NoWarn> | ||
<RestoreOutputPath>$(MSBuildThisFileDirectory)\System.Runtime\ref\</RestoreOutputPath> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this going to System.Runtime\ref? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a remnant of changes I brought in from Jose's original changeset - the location pointed to here needs a project.assets.json file, which I also added in this change. I'm not married to that particular location There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is wrong, I had it like this as part of my changes only when I was iterating on some changes and wanted to verify that stuff was being restored correctly. This should be removed. |
||
<IsSDKProject>true</IsSDKProject> | ||
</PropertyGroup> | ||
<PropertyGroup> | ||
<CSharpCoreTargetsPath>$(MSBuildExtensionsPath32)\Roslyn\Microsoft.CSharp.Core.targets</CSharpCoreTargetsPath> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need to explicitly set these here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is also a remnant I took from Jose's original changeset - I haven't tried running a build without it yet. |
||
<VisualBasicCoreTargetsPath>$(MSBuildExtensionsPath32)\Roslyn\Microsoft.VisualBasic.Core.targets</VisualBasicCoreTargetsPath> | ||
</PropertyGroup> | ||
</Project> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
<?xml version="1.0" encoding="utf-8"?> | ||
<Project InitialTargets="AddSkipGetTargetFrameworkToProjectReferences"> | ||
<Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildProjectDirectory), dir.targets))\dir.targets" /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What are these targets for? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is just what imports the parent directory's dir.targets. We used to have this per project, and we now have it only once on the common file. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we instead of import dir.targets, rename dir.targets to Directory.Build.Targets? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would be fine with that, though it would probably confuse some devs at first There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Directory.Build.Targets is a new feature of msbuild that some repos under dotnet/aspnet have started to use already: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you think I should rename all dir.props/dir.targets to Directory.Build.props/targets? @joperezr is there documentation on how SDK projects automatically find files named as such? Will there be collisions if multiple files share that name across different parts of the directory tree? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Check out https://docs.microsoft.com/en-us/visualstudio/msbuild/customize-your-build There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. dir.props and dir.targets are maltilayered in many repos (they are present at multiple directory levels and recursively each one includes higher level projects). Directory.Build.props/.targets are included once - after first include higher level ones will be skipped. This may cause some problems unless recursive include logic from dirs.props is applied to Directory.Build.* files as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I expect we will recursively include them if we nest them. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this is expected with Directory.Build.*. The first one gets imported, and then if there are parent props/targets files, they need to be explicitly included up the chain. This allows props/target authors to get the ordering right, which is important especially in props files. We do this in the dotnet/machinelearning repo: https://github.com/dotnet/machinelearning/blob/master/src/Directory.Build.props#L3 |
||
<Target Name="_CheckForInvalidConfigurationAndPlatform" /> | ||
<Target Name="_CheckForUnsupportedTargetFramework" /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps it is part of your buildtools changes but how are we blocking the import of the csharp targets? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't explicitly done this yet, maybe a change I will have to implement in buildtools. |
||
<Target Name="RunResolvePackageDependencies" /> | ||
<Target Name="AddSkipGetTargetFrameworkToProjectReferences" Condition="'@(ProjectReference)' != ''"> | ||
<ItemGroup> | ||
<ProjectReference> | ||
<SkipGetTargetFrameworkProperties>true</SkipGetTargetFrameworkProperties> | ||
</ProjectReference> | ||
</ItemGroup> | ||
</Target> | ||
</Project> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eerhardt are you aware of any way we can avoid setting this string in all our projects? We are very likely going to need to move to another SDK as part of our arcade repo and it would be great if we could just change it in one place in the repo if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on dotnet/msbuild#1493, we can use
<Import>
semantics, because inProject Sdk="$(prop)"
,prop
does not be evaluated, nor in<Sdk Name="$(prop)">
. Alternatively, we can implementISdkResolver
when it is implemented in MSBuild. :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can do something like the following commit:
eerhardt/machinelearning@ed3c014
This allows my .csproj files to be built from the command line using
dotnet build
.However, VS 2017 throws an error trying to load this project:
@davkean - Any thoughts on this VS error?
@weshaggard - honestly, I think attempting to put the SDK name in a single place is going to be an uphill battle, and I'm not sure it is worth the effort. Yes, it will change one, maybe two more times. But IMO the work and effort of find/replace in a few hundred files is less than trying to do things that the tools weren't designed to support. And note that you need 2 imports in every .csproj with my approach above (props on top, targets on bottom).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it isn't a big deal. We should however be sure to keep the version out of all the project files as we we need to be able to override that in some cases like source-build so lets keep the version in the global.json file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error above indicates that you are not importing Microsoft.Common.targets and indirectly Microsoft.Managed.DesignTime.targets. You can verify this by doing a
msbuild /pp
on the project.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can import the SDK in a central props/targets, but then the central props/targets can't be
Directory.Build.*
. Chicken and egg. Sdk imports common targets and common targets importsDirectory.Build.*
If you use explicit dir.props/dir.targets, then you can do this: nguerrera/sdkimportrepro@6d34578
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see that @eerhardt explicitly imported
Directory.Build.*
and then turned off the magic import. That should have worked too. My above commit with different name for explicit directory props/targets is working fine in VS. Let me try @eerhardt's approach in that sandbox.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nguerrera/sdkimportrepro@ab90e62 works fine in VS for me too. I cannot repro what @eerhardt is seeing with that approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried it on today's daily dogfood build, and am no longer seeing the error. The project loads and builds correctly in VS.