-
Notifications
You must be signed in to change notification settings - Fork 65
Update baseline infra with specifics for deployment-tools repo #1
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
Update baseline infra with specifics for deployment-tools repo #1
Conversation
eng/Signing.props
Outdated
| <ItemGroup Condition="'$(SignR2RBinaries)' == 'true'"> | ||
| <ItemsToSign Include="$(CrossGenRootPath)**/*.dll" /> | ||
| <!-- Sign ClickOnce binaries. --> | ||
| <!--ItemsToSign Include="$(ArtifactsBinDir)Mage/**/*.dll" /--> |
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.
Delete?
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.
Fixing.
eng/Subsets.props
Outdated
| <SubsetName Include="Installer.DepProjs" Description="The dependency projects. These gather shared framework files and run crossgen on them to turn them into ready-to-run (R2R) assemblies for the current platform." /> | ||
| <SubsetName Include="Installer.PkgProjs" Description="The packaging projects. These produce NETCoreApp assets: NuGet packages, installers, zips, and Linux packages." /> | ||
| <SubsetName Include="Bundles" Description="The shared framework bundle installer projects. Produces .exe installers for Windows." /> | ||
| <SubsetName Include="Installers" Description="Generates additional installers. This produces the shared frameworks and their installers." /> |
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.
| <SubsetName Include="Installers" Description="Generates additional installers. This produces the shared frameworks and their installers." /> | |
| <SubsetName Include="Installers" Description="Generates installers. This produces the deployment tooling installers." /> |
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.
Will fix - thanks.
eng/Subsets.props
Outdated
| <ItemGroup Condition="$(_subset.Contains('+installers+'))"> | ||
| <InstallerProjectToBuild Include="$(InstallerProjectRoot)pkg\packaging\installers.proj" BuildInParallel="false" /> | ||
| <InstallerProjectToBuild Include="$(InstallerProjectRoot)pkg\packaging\vs-insertion-packages.proj" BuildInParallel="false" /> | ||
| <InstallerProjectToBuild Include="$(InstallerProjectRoot)**\*.proj" BuildInParallel="false" /> |
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.
BuildInParallel="false" is a workaround that shouldn't be necessary: dotnet/runtime#494 (comment).
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.
Will fix.
eng/liveBuilds.targets
Outdated
| If this is running and the output RID is not the same as the targeted RID, resolve live assets | ||
| for the targeted RID, if available. This is used to gather asset metadata for the platform | ||
| manifest. In CI (multi-machine lab) builds, CoreCLR and Libraries artifacts are all downloaded | ||
| manifest. In CI (multi-machine lab) builds, ClickOnce and other artifacts are all downloaded | ||
| onto the current machine from all platforms for the Installer portion of the build. |
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 this actually necessary (and is it performed) for deployment-tools?
eng/liveBuilds.targets
Outdated
| <!-- | ||
| Ensure artifacts exist for the more advanced paths. If the configuration is '*', don't emit | ||
| these errors: it isn't a local dev scenario. | ||
| --> |
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.
Orphaned comment
src/clickonce/Directory.Build.props
Outdated
| <DisableCrossgen>false</DisableCrossgen> | ||
| <!-- Disable cross-gen on FreeBSD for now. This can be revisited when we have full support. --> | ||
| <DisableCrossgen Condition="'$(TargetOS)'=='FreeBSD'">true</DisableCrossgen> | ||
| <OutputVersionBadge>$(AssetOutputPath)sharedfx_$(OutputRid)_$(Configuration)_version_badge.svg</OutputVersionBadge> |
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.
Remove this too? Old usage is in pkg/packaging/installers.proj: https://github.com/dotnet/runtime/blob/5f09f33bc547e7c6733ab89e05baf9e860079bba/src/installer/pkg/packaging/installers.proj#L182
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.
Yeah - not needed, will remove.
| endif() | ||
|
|
||
| add_subdirectory(cli) | ||
| set(APP_HOST_LIB_DIR ${DOTNET_PACKS_DIR}/Microsoft.NETCore.App.Host.win-${CLR_CMAKE_HOST_ARCH}/${NET_CORE_PKG_VER}/runtimes/win-${CLR_CMAKE_HOST_ARCH}/native) |
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 seems fragile... I think ideally you would resolve it via MSBuild (from global install or local--whatever's running) to get the right arch + version + location.
ProcessFrameworkReferences sets up this item with dotnet publish -r win-x64 --self-contained false /bl on a simple console app proj. Might be the best way to go:
AppHost
PackageDirectory = C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Host.win-x64\3.1.2
Path = C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Host.win-x64\3.1.2\runtimes\win-x64\native\apphost.exe
PathInPackage = runtimes\win-x64\native\apphost.exe
RuntimeIdentifier = win-x64
I think it's reasonable to track this as a future improvement and use what you already have to get the repo off the ground.
(On the other hand... I don't actually see how this cmake var is used.)
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 will be used with a project that will be added as soon as infra is ready. I will open an issue to improve this.
src/clickonce/native/build.proj
Outdated
| <Exec Command="$(MSBuildProjectDirectory)\build.sh $(BuildArgs)" IgnoreStandardErrorWarningFormat="true"/> | ||
| </Target> | ||
|
|
||
| <Target Name="BuildCoreHostWindows" |
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.
Target name inaccurate? (Maybe just make it more generic.)
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.
Thanks - will fix.
Changes in infra specific to deployment-tools repo and projects it would build.
Includes some initial changes in Readme and Contributing files - more changes will come in next PR.