-
Notifications
You must be signed in to change notification settings - Fork 4.9k
external/dir.props: remove Portable condition #21032
Changes from all commits
e073c88
cb42d9b
092e61e
7da461b
ec753ab
c256f1a
9f14fb0
7934e18
d64ea1c
4d7bc17
1bb4d6c
22e9d82
0f7d4e9
fc18ae8
85119e4
31f6398
da2bf91
f105cc9
4196d22
97ef94f
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 |
|---|---|---|
|
|
@@ -31,11 +31,17 @@ | |
| "defaultValue": false | ||
| }, | ||
| "RuntimeOS": { | ||
| "description": "The RuntimeOS used for building and restoring native OS dependent assets (i.e. RID specific binaries)", | ||
| "description": "The RuntimeOS of the build system.", | ||
| "valueType": "property", | ||
| "values": ["win7", "osx.10.10", "ubuntu.14.04", "ubuntu.16.04", "etc-other-rid-based-os-names"], | ||
| "defaultValue": "" | ||
| }, | ||
| "PackageRID": { | ||
| "description": "The RID of the target package.", | ||
|
Member
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 is the target package in this case? I assume you are implying that it is the RID of what you are building, however I see you a also using it in restoring dependencies.
Member
Author
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 the target RID you are building for. |
||
| "valueType": "property", | ||
| "values": ["win7-x64", "ubuntu.14.04-x64"], | ||
| "defaultValue": "" | ||
| }, | ||
| "PortableBuild": { | ||
| "description": "Indicates if this is a portable build.", | ||
| "valueType": "property", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,11 +2,7 @@ | |
| <Project ToolsVersion="14.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003" DefaultTargets="Build"> | ||
| <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" /> | ||
| <PropertyGroup> | ||
| <!-- support cross-targeting by choosing a RID to restore when running on a different machine that what we're build for --> | ||
| <NugetRuntimeIdentifier Condition="'$(OSGroup)' == 'Windows_NT' AND '$(RunningOnUnix)' == 'true'">win-x64</NugetRuntimeIdentifier> | ||
| <NugetRuntimeIdentifier Condition="'$(OSGroup)' == 'Unix' AND '$(RunningOnUnix)' != 'true'">linux-x64</NugetRuntimeIdentifier> | ||
| <NugetRuntimeIdentifier Condition="'$(TargetGroup)' == 'uap'">win10-$(ArchGroup)</NugetRuntimeIdentifier> | ||
| <NugetRuntimeIdentifier Condition="'$(TargetGroup)' == 'uapaot'">win10-$(ArchGroup)-aot</NugetRuntimeIdentifier> | ||
| <NugetRuntimeIdentifier>$(PackageRID)</NugetRuntimeIdentifier> | ||
|
Member
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'm not sure I understand why we are using PackageRID here? It should be based on the OS you are running on as these assets are primarily about running the tests on the build you are using.
Member
Author
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. Doesn't this runtime get included in the built package.
Member
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. No the runtime doesn't get used for anything beyond running tests in this repo. It doesn't get included in any packages that we produce in corefx.
Member
Author
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 see. I thought Microsoft.NETCore.App is built by corefx? This package contains the runtime?
Member
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. That package is assembled in the core-setup repo, which consumes the runtime packages and the private corefx package (Microsoft.Private.CoreFx.NETCoreApp) but there are no packages that build in corefx that contain the runtime.
Member
Author
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. runtime.depproj pre-PR looks like it doesn't match the host but the target.
Member
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 that is a specific scenario where someone is trying to build for Windows while running on Unix. I guess I did overstate when I mentioned that this is only used for testing. There are some scenarios were we use the assets for building the product in the case of a ReferenceFromRuntime reference (https://github.com/dotnet/corefx/blob/master/referenceFromRuntime.targets#L3) for projects that directly compile against System.Private.CoreLib. Unfortunately there are some slightly different APIs across the different OS's. So the most common scenario is folks that want to build and run on the device the are on but in some cases folks will want to build for another runtime but not run locally but instead send them to another machine to run. So that is more of a target/PackageRID as opposed to Host/ToolRuntimeRID. So with that said I think using PackageRID here is appropriate. |
||
| <RidSpecificAssets>true</RidSpecificAssets> | ||
| </PropertyGroup> | ||
| <PropertyGroup Condition="'$(TargetGroup)' == 'uapaot'"> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,20 +2,13 @@ | |
| <Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003"> | ||
| <Import Project="..\dir.props" /> | ||
|
|
||
| <PropertyGroup> | ||
| <_runtimeOSVersionIndex>$(RuntimeOS.IndexOfAny(".-0123456789"))</_runtimeOSVersionIndex> | ||
| <_runtimeOSFamily Condition="'$(_runtimeOSVersionIndex)' != '-1'">$(RuntimeOS.SubString(0, $(_runtimeOSVersionIndex)))</_runtimeOSFamily> | ||
| </PropertyGroup> | ||
|
|
||
| <!-- Packages opt-in to automatic RID-specific builds by placing a *.RID.props next to their project | ||
|
Member
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.
Member
Author
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 don't understand what mean. My guess (which may be off) is that moving PackageRID to the top is causing the NETFX build failures. <ItemGroup>
<BuildRID Include="@(OfficialBuildRID)" Exclude="$(PackageRID)"/>
<BuildRID Include="$(PackageRID)">
<Platform Condition="'$(ArchGroup)' == 'x64'">amd64</Platform>
<Platform Condition="'$(ArchGroup)' != 'x64'">$(ArchGroup)</Platform>
</BuildRID>
</ItemGroup>
Member
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. Please read the comment. Prior to your change PackageRID was treated optional and meant to signify opt-in to RID-specific packages. Now you're setting PackageRID for everything so it can no longer be opt-in. I suggest you change this: <!-- Packages opt-in to automatic RID-specific builds by placing a *.RID.props next to their project
that defines the following:
PackageRID property: the RID which we should be building for
OfficialBuildRID item: all RIDs targeted by the package -->
<Import Project="$(MSBuildProjectDirectory)\*.rids.props" />
<!-- create the "BuildRID" item which is the set of all supported RIDs, with metadata.
We'll add a RID for the current platform even if it isn't in the officially supported set -->
<ItemGroup>
<BuildRID Include="@(OfficialBuildRID)" Exclude="$(PackageRID)"/>
<BuildRID Include="$(PackageRID)">
<Platform Condition="'$(ArchGroup)' == 'x64'">amd64</Platform>
<Platform Condition="'$(ArchGroup)' != 'x64'">$(ArchGroup)</Platform>
</BuildRID>
</ItemGroup>To this: <!-- Packages opt-in to automatic RID-specific builds by placing a *.RID.props next to their project
that defines the OfficialBuildRID item: all RIDs targeted by the package -->
<Import Project="$(MSBuildProjectDirectory)\*.rids.props" />
<!-- create the "BuildRID" item which is the set of all supported RIDs, with metadata.
We'll add a RID for the current platform even if it isn't in the officially supported set -->
<ItemGroup Condition="'@(OfficialBuildRID)' != ''">
<BuildRID Include="@(OfficialBuildRID)" Exclude="$(PackageRID)"/>
<BuildRID Include="$(PackageRID)">
<Platform Condition="'$(ArchGroup)' == 'x64'">amd64</Platform>
<Platform Condition="'$(ArchGroup)' != 'x64'">$(ArchGroup)</Platform>
</BuildRID>
</ItemGroup> |
||
| that defines the following: | ||
| PackageRID property: the RID which we should be building for | ||
| OfficialBuildRID item: all RIDs targeted by the package --> | ||
| that defines the OfficialBuildRID item: all RIDs targeted by the package --> | ||
| <Import Project="$(MSBuildProjectDirectory)\*.rids.props" /> | ||
|
|
||
| <!-- create the "BuildRID" item which is the set of all supported RIDs, with metadata. | ||
| We'll add a RID for the current platform even if it isn't in the officially supported set --> | ||
| <ItemGroup> | ||
| <ItemGroup Condition="'@(OfficialBuildRID)' != ''"> | ||
| <BuildRID Include="@(OfficialBuildRID)" Exclude="$(PackageRID)"/> | ||
| <BuildRID Include="$(PackageRID)"> | ||
| <Platform Condition="'$(ArchGroup)' == 'x64'">amd64</Platform> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,7 +10,6 @@ | |
| <LKGSharedFrameworkExtractPath>$(RefRootPath)sharedFrameworkZip.LKG/shared/Microsoft.NETCore.App/$(_LKGSharedFrameworkVersion)</LKGSharedFrameworkExtractPath> | ||
| <_RunTestsScriptName Condition="'$(RunningOnUnix)' != 'true'">CloneAndRunTests.cmd</_RunTestsScriptName> | ||
| <_RunTestsScriptName Condition="'$(RunningOnUnix)' == 'true'">CloneAndRunTests.sh</_RunTestsScriptName> | ||
| <_PublishRID>$(RuntimeOS)-$(ArchGroup)</_PublishRID> | ||
|
Member
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. All I would change is the definition of _PublishRID here to use TargetRID instead, but all of the other changes on this file and the RestoreSDKProject should be undone. |
||
| </PropertyGroup> | ||
|
|
||
| <Target Name="CreateScriptToDownloadSharedFrameworkZip"> | ||
|
|
@@ -57,16 +56,16 @@ | |
|
|
||
| <Target Name="BuildPackagesProject"> | ||
| <PropertyGroup> | ||
| <_RestorePackagesCommand>"$(DotnetToolCommand)" restore $(MSBuildThisFileDirectory)/RestoreSDKProject/RestoreSDKProject.csproj /p:_PublishRID=$(_PublishRID) /p:_RuntimeFrameworkVersion=$(MicrosoftNETCoreAppPackageVersion) /p:CoreFxExpectedPrerelease=$(CoreFxExpectedPrerelease)</_RestorePackagesCommand> | ||
|
Member
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 still need to pass this in since the project we are building doesn't import dir.props and doesn't know about the regular properties like |
||
| <_PublishPackagesProjectCommand>"$(DotnetToolCommand)" publish $(MSBuildThisFileDirectory)/RestoreSDKProject/RestoreSDKProject.csproj /p:_PublishRID=$(_PublishRID) /p:_MetaPackageDestinationFolder=$(SharedFrameworkValidationRefPath) -f netcoreapp2.0 -r $(_PublishRID) -o $(SharedFrameworkValidationRuntimePath)</_PublishPackagesProjectCommand> | ||
| <_RestorePackagesCommand>"$(DotnetToolCommand)" restore $(MSBuildThisFileDirectory)/RestoreSDKProject/RestoreSDKProject.csproj /p:_PackageRID=$(PackageRID) /p:_RuntimeFrameworkVersion=$(MicrosoftNETCoreAppPackageVersion) /p:CoreFxExpectedPrerelease=$(CoreFxExpectedPrerelease)</_RestorePackagesCommand> | ||
| <_PublishPackagesProjectCommand>"$(DotnetToolCommand)" publish $(MSBuildThisFileDirectory)/RestoreSDKProject/RestoreSDKProject.csproj /p:_PackageRID=$(PackageRID) /p:_MetaPackageDestinationFolder=$(SharedFrameworkValidationRefPath) -f netcoreapp2.0 -o $(SharedFrameworkValidationRuntimePath) -r $(PackageRID)</_PublishPackagesProjectCommand> | ||
| </PropertyGroup> | ||
| <Exec Command="$(_RestorePackagesCommand)" StandardOutputImportance="Low" /> | ||
| <Exec Command="$(_PublishPackagesProjectCommand)" StandardOutputImportance="Low" /> | ||
| </Target> | ||
|
|
||
| <Target Name="BuildDependenciesProject"> | ||
| <MSBuild Projects="../../external/SharedFrameworkValidation/harvestPackages.SharedFramework.depproj" | ||
| Properties="_PublishRID=$(_PublishRID)" | ||
|
Member
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 need to re-add this back because of my previous comment. |
||
| Properties="_PackageRID=$(PackageRID)" | ||
|
Member
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. Not 100% convinced that the property rename is required, but I'm ok with the changes.
Member
Author
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. Thanks. The rename is to make more clear what RIDs are for the build system and which are for the package. |
||
| ContinueOnError="ErrorAndStop" /> | ||
| </Target> | ||
|
|
||
|
|
||
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.
This is meant to represent what we're running on.