-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Add Features projects to SourceBuild #57277
Changes from 10 commits
674f43e
32c506a
0569f3d
919237c
8b1f3cb
8fa8561
4c513d4
d6ceb3a
3d8ae85
df8a913
76d7cf8
da94933
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 |
---|---|---|
|
@@ -27,9 +27,9 @@ | |
<ItemGroup> | ||
<PackageReference Include="System.Collections.Immutable" Version="$(SystemCollectionsImmutableVersion)" /> | ||
<PackageReference Include="Moq" Version="$(MoqVersion)" /> | ||
<PackageReference Include="Microsoft.Build" Version="$(MicrosoftBuildVersion)" /> | ||
<PackageReference Include="Microsoft.Build.Framework" Version="$(MicrosoftBuildFrameworkVersion)" /> | ||
<PackageReference Include="Microsoft.Build.Tasks.Core" Version="$(MicrosoftBuildTasksCoreVersion)" /> | ||
<PackageReference Include="Microsoft.Build" Version="$(RefOnlyMicrosoftBuildVersion)" /> | ||
<PackageReference Include="Microsoft.Build.Framework" Version="$(RefOnlyMicrosoftBuildFrameworkVersion)" /> | ||
<PackageReference Include="Microsoft.Build.Tasks.Core" Version="$(RefOnlyMicrosoftBuildTasksCoreVersion)" /> | ||
Comment on lines
+30
to
+32
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 all uses of 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. We should update BuildBoss to require the attribute if a RefOnly package is used. 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. Would The name of these properties is misleading in this context for someone working on the Microsoft build. Really, any name that indicates these versions are "pinned" would do fine, if this is a concern. In non-unit-test projects, |
||
<PackageReference Include="System.Threading.Tasks.Dataflow" Version="$(SystemThreadingTasksDataflowVersion)" /> | ||
</ItemGroup> | ||
<ItemGroup> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,3 @@ | ||
<Project> | ||
<Import Project="$([MSBuild]::GetPathOfFileAbove('Directory.Build.props', '$(MSBuildThisFileDirectory)../'))" /> | ||
<PropertyGroup> | ||
JoeRobich marked this conversation as resolved.
Show resolved
Hide resolved
|
||
<ExcludeFromSourceBuild>true</ExcludeFromSourceBuild> | ||
</PropertyGroup> | ||
</Project> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
<Project> | ||
<Import Project="$([MSBuild]::GetPathOfFileAbove('Directory.Build.props', '$(MSBuildThisFileDirectory)../'))" /> | ||
<PropertyGroup> | ||
<ExcludeFromSourceBuild>true</ExcludeFromSourceBuild> | ||
</PropertyGroup> | ||
</Project> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
<Project> | ||
<Import Project="$([MSBuild]::GetPathOfFileAbove('Directory.Build.props', '$(MSBuildThisFileDirectory)../'))" /> | ||
<PropertyGroup> | ||
<ExcludeFromSourceBuild>true</ExcludeFromSourceBuild> | ||
</PropertyGroup> | ||
</Project> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
<Project> | ||
<Import Project="$([MSBuild]::GetPathOfFileAbove('Directory.Build.props', '$(MSBuildThisFileDirectory)../'))" /> | ||
<PropertyGroup> | ||
<ExcludeFromSourceBuild>true</ExcludeFromSourceBuild> | ||
</PropertyGroup> | ||
</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.
Is there a version of MSBuild we can be referencing that doesn't have this caveat?
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.
@dagood is MSBuild 16.11 compatible?
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 thought Davis did a good job of explaining the situation here: #57277 (comment). If you haven't read that, check it out.
If you name this property
MicrosoftBuildVersion
, when building for source-build that property will automatically get overriden to point to the currently built version of Microsoft.Build.nupkg that was being built during source-build. This means this property would point to version17.0-previewXXX
.However, Roslyn's build doesn't succeed when trying to build against the current source-built MSBuild nuget packages (probably because your tasks projects are building for net472 and the source-build MSBuild packages didn't at the time - that was fixed yesterday dotnet/installer#12472).
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'd have to use
17.0.0
(current latest that is being included in 6.0.100 SDK) and set up aeng/Version.Details.xml
dependency and subscription on MSBuild to make sure it's always up to date. Any version other than the one being shipped in the SDK needs this kind of ref-only handling.My understanding is that this dependency is most likely stuck on an old version because it needs to be compatible with old VS toolsets, anyway--so moving to a non-16.5.0 & non-17.0.0 version wouldn't work from an MSBuild perspective, let alone source-build's.
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.
If MSBuild 16.11 is source build compatible, I will update our dependency and revert the 'RefOnly' changes in a follow up.
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.
It's not, only an actively updated
17.0.0[-*]
would be.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.
Maybe "caveat" is just too strong. Ref-only usage is everywhere, and it would be nice for source-build to get rid of it and have current references everywhere, but we expect old-ref-usage to stay in a lot of places because of the requirements the Microsoft build needs to fulfill. (E.g. NuGet packages need to be usable by people who are using old framework versions; VS has some complex compat requirements with the toolset it ships with.)
To me, this code comment is just a description of a normal process that is fine to have here. I filed dotnet/source-build#2546 so it can be hopefully be replaced with a link to a doc that describes this in more detail and with more context.
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.
@dagood @JoeRobich I'm not sure how well we support this library being used downlevel in that way -- it's not something we have automated test coverage for in any case. I'm not sure if this breaks folks that might have depended on our package and were also relying on us to have the dependency of some version if they didn't have another VS version on the box.