Installer updates (Merge to SDK Changeset) #17959

MiYanni · 2023-12-13T02:15:49Z

Sibling branch: https://github.com/dotnet/sdk/tree/feature/installer-merge

Summary

The work in this PR is in preparation for merging this repo into the SDK repo. The changes I've made here will be the general format that the files will be when moved to the SDK repo. You can take a look a the linked sibling branch to see these files and how they'll be in the SDK repo.

Because this endeavor is taking a phased approach, it will be easier to maintain updates to these files in the SDK repo if the files in the Installer repo match a similar format/form. Therefore, I believe it makes the most sense to merge this PR into the Installer repo, even though we plan on phasing out this repo (at a later date).

High-Level Changes

Formatting and cleanup of most of the files in the repo
Usage of Microsoft.Build.NoTargets for projects that don't require building assemblies
Minor changes in some of the core-sdk-tasks code to use an explicit type instead of ValueTuples
Simplification of the finalizer build process
Reduction in folder nesting for src (some projects/files combined into other project folders)
Removed LayoutTool.csproj and used DownloadPackage.csproj with the added logic for tool acquisition in BundledDotnetTools.targets

Things of Note

When comparing the differences between these changes and the SDK repo's branch:
- This repo doesn't use Directory.Packages.props, so versioning of packages is explicit within projects
- This repo doesn't use global usings, so there are many more using statements in the source files
~~Currently, there are manual dependency version changes. This is only done for the purposes of comparing the output between the repos. These changes will be reverted prior to merge.~~
- Fixed.
~~Some TODOs exist. These will be removed prior to merge.~~
- Fixed.
There will be conflicts as syncing updates is a process I need to do between this repo and the SDK branch.

Finalizer Changes Details

The current process for building finalizer proceeds as such:

eng/Build.props builds eng/version.csproj and eng/native.proj
- eng/version.csproj's only purpose is to call the GenerateSdkVersionFile target. Additionally, this project is only used in the finalizer build process.
- eng/native.proj's only purpose is to call Native.sln and pass Platform (set using the $(Architecture) property) to the solution.
Native.sln only contains a single project, src/finalizer_shim/finalizer_shim.csproj
finalizer_shim.csproj provides the data to the CMake SDK to build the src/finalizer/finalizer.proj and runs the build of that project. Also, it copies WixSdk contents to use in the build.
finalizer.proj sets the CMakeLists.txt file and defines the Platform variable
CMakeLists.txt contains the actual build logic for creating finalizer.exe

I've simplified the build process as such:

eng/Build.props builds src\finalizer\finalizer-build.csproj
finalizer-build.csproj contains the GenerateSdkVersionFile target, provides the CMake SDK data, copies WixSdk contents, and builds finalizer.nativeproj
finalizer.nativeproj uses TreatAsLocalProperty="Platform" to set the property for its build, sets the CMakeLists.txt file, and defines the Platform and ArtifactsDir variables
CMakeLists.txt contains the actual build logic for creating finalizer.exe

Because of this, I was able to remove:

eng/version.csproj
eng/native.proj
Native.sln

Since the whole process only stems from one project, finalizer-build.csproj, this made it easier to work with in the SDK repo.

Test Builds

…ession bodied properties to solution.

…s. Set var suggestion to none.

…r the 4.7.2 build, so changed it to class.

…be necessary. Manual formatting cleanup for targets in the redist project.

…he target from LayoutTool.csproj directly into where it was used in BundledDotnetTools.targets. Added comments to DownloadPackage.csproj.

…der redist. Added more comments to DownloadPackage.csproj.

…an up formatting in SdkTests.csproj. Removed the empty <TargetPath></TargetPath> from SdkTests.csproj to allow loading in VS.

…teLocator.csproj projects. Added the projects back to the SLN so they build using the proper configuration (debug/release).

…jects only ever build with the default configuration (Debug) since it is irrelevant for packaging. Updated the paths used when creating the MSIs, which uses the output of these to create NuGet packages.

…ller-merge-update # Conflicts: # eng/Version.Details.xml # eng/Versions.props # global.json # src/redist/targets/Crossgen.targets # src/redist/targets/GenerateBundledVersions.targets # src/redist/targets/GenerateLayout.targets # src/redist/targets/GenerateMSIs.targets

…from SdkTests as it hasn't existed in years...?

…e item group properly).

MiYanni · 2024-02-28T19:31:40Z

src/redist/targets/GenerateMSIs.targets

-      <SdkStableFileIdForApphostTransform>$(MSBuildThisFileDirectory)packaging/windows/clisdk/stablefileidforapphosttransform.xslt</SdkStableFileIdForApphostTransform>
-      <SdkGenerateBundlePowershellScript>$(MSBuildThisFileDirectory)packaging/windows/clisdk/generatebundle.ps1</SdkGenerateBundlePowershellScript>
-      <GenerateNupkgPowershellScript>$(MSBuildThisFileDirectory)packaging/windows/clisdk/generatenupkg.ps1</GenerateNupkgPowershellScript>
+      <SdkPkgSourcesRootDirectory>$(MSBuildThisFileDirectory)../packaging/windows</SdkPkgSourcesRootDirectory>


Added this to simply all of these path properties.

src/redist/targets/GenerateMSIs.targets

MiYanni · 2024-02-28T19:39:55Z

src/redist/targets/GeneratePKG.targets

A few of the targets files had this problem where the content in the file was indented incorrectly. It makes it look like I changed a lot, but all I did was fix the tab alignment.

MiYanni · 2024-02-28T19:41:43Z

src/redist/targets/LayoutTool.csproj

Removed in favor of DownloadPackage.csproj instead.

MiYanni · 2024-02-28T20:01:09Z

test/Microsoft.DotNet.Tools.Tests.Utilities/RepoDirectoriesProvider.cs

-                    string configuration = new DirectoryInfo(AppContext.BaseDirectory).Parent.Name;
+                    // https://stackoverflow.com/a/60545278/294804
+                    var assemblyConfigurationAttribute = typeof(RepoDirectoriesProvider).Assembly.GetCustomAttribute<AssemblyConfigurationAttribute>();
+                    string configuration = assemblyConfigurationAttribute?.Configuration;


Simplification as the parent directory's name is not the same between the Installer repo and SDK repo. The point of this is to simply get the build configuration. So, I directly get that information from the assembly information.

MiYanni · 2024-02-28T20:05:26Z

test/SdkTests/SdkTests.csproj

-    <Exec Command="$(ToolRunPrefix)$(InstallToolCommand)"
-      WorkingDirectory="$(TestLocalToolFolder)"
-      EnvironmentVariables="DOTNET_CLI_HOME=$(DOTNET_CLI_HOME)"/>

-    <!--<Exec Command="$(ToolRunPrefix)dotnet tool restore"
+    <Exec Command="$(ToolRunPrefix)$(InstallToolCommand)"
      WorkingDirectory="$(TestLocalToolFolder)"
-      EnvironmentVariables="DOTNET_CLI_HOME=$(DOTNET_CLI_HOME)"/>-->
+      EnvironmentVariables="DOTNET_CLI_HOME=$(DOTNET_CLI_HOME)" />


The diffing here looks weird but all I did was remove commented code.

MiYanni · 2024-02-28T20:07:12Z

test/SdkTests/SdkTests.csproj

-  <PropertyGroup>
-    <TargetPath></TargetPath>
-    <!-- Prevent projects referencing this from trying to pass us to the compiler -->
-  </PropertyGroup>


This makes the project inoperable in Visual Studio as the project system requires that TargetPath have a value.

Should that be conditioned on the IsBuildingInVisualStudio property or something?

Should that be conditioned on the IsBuildingInVisualStudio property or something?

I'll respond to your question with a question, does <TargetPath></TargetPath> have a purpose? The comment under it implies that SdkTests.csproj is referenced by other projects, but it isn't. Nothing references this project.

MiYanni · 2024-02-28T20:08:07Z

test/SdkTests/SdkTests.csproj

@@ -267,12 +250,10 @@
  <!-- Group always attempts resolve runtime, regardless of <CopyNuGetImplementations>-->
  <Target Name="GetReferenceAssemblyPaths" />
  <!-- Don't go looking for framework reference assemblies-->
-  <Target Name="GetFrameworkPaths"  />
-  <!-- ^ -->


MiYanni · 2024-02-28T20:09:40Z

test/SdkTests/TestConfig.xml

@@ -37,7 +37,6 @@
    <Method Name="Microsoft.NET.Publish.Tests.GivenThatWeWantToPublishASelfContainedApp.It_can_make_a_Windows_GUI_exe" />
    <Class Name="Microsoft.NET.Publish.Tests.GivenThatWeWantToPublishASingleFileApp" />
    <Class Name="Microsoft.NET.Publish.Tests.GivenThatWeWantToPublishAWebApp" />
-    <Class Name="Microsoft.NET.Publish.Tests.GivenThatWeWantToRunCrossgen" />


This test doesn't exist anymore. I found it existing in 2019 but I didn't walk the entire file history to see when it was deleted.

This sort of thing will hopefully become easier once everything is one repo

Well, this file isn't being migrated since it was for running SDK tests in this repo. I went through and confirmed these tests being in the SDK repo already, so I don't need to do anything with the entire SdkTests project. This cleanup just came out of that review.

…ssary. Updated or removed TODO comments. Fixed some tabbing from merging. Fixed a compilation bug if this code is ever built against 4.5.1(????)

…et prior to calling the Arcade SDK, so the OutputPath properly includes the Architecture folder.

joeloff · 2024-03-07T16:03:45Z

src/core-sdk-tasks/BuildFPMToolPreReqs.cs

@@ -6,7 +6,6 @@
 using Microsoft.Build.Utilities;
 using Microsoft.Build.Framework;
 using System.IO;


Since you're cleaning up some of the code, might as well sort the usings too

So, I didn't run the same formatters/analyzers like I did in the SDK repo on this code. Here, I likely just did "Remove unused usings". However, the point is kinda moot when you look at this file in the SDK PR:
https://github.com/dotnet/sdk/pull/38804/files#diff-ea4fa261d2db3cc7e1e241b5f6ac3a003ff19fdde37c09ddb089b7467277160dR4

There's only 1 using because all of these are in the global usings of that repo. So... 🙃

joeloff · 2024-03-07T16:06:15Z

src/core-sdk-tasks/BuildFPMToolPreReqs.cs

@@ -183,14 +179,11 @@ private string BuildCmdParameters(ConfigJson configJson, string package_version)

            // Map all the payload directories as they need to install on the system 
            if (configJson.Install_Root != null)


Time for me to rant. I think our general style guidelines have always been to have braces for if statements, though if you really want to you may be able to use conditionals and discard statements. Not a big issue since this code has been around for a long time I think

Oh definitely. But there's code in the SDK repo that doesn't follow ANY guidelines... that was written in 2006, originally from .NET Framework. I think there are more formatting guidelines I'd like to turn on in the SDK repo, but I don't have a complete list yet. I'll mention I didn't do "complete" cleanup to the code here. It was more like spot checks and some automatic tasks in VS. I did a lot of these in December when I was trying to understand the scope and projects in this repo for migration.

joeloff · 2024-03-07T17:07:08Z

src/finalizer/finalizer.nativeproj

+    <CMakeDefines Include="Platform" Value="$(Platform)" />
+    <CMakeDefines Include="ArtifactsDir" Value="$(ArtifactsDirForCMake)" />
+    <!-- Include this item below to create a binlog for the native build. -->
+    <!-- <CMakeNativeToolArguments Include="/bl:$(ArtifactsDirForCMake)native.binlog" /> -->


Any reason why this is commented out? Do we want it enabled by default or otherwise hide it behind a conditional property? Having binlogs for everything is useful when investigating build failures I think

It was there in the file already (meaning, it was commented out in the file) and I figured that you put it there for yourself. Lol. I didn't look at the blame on it, though. I haven't actually tested with it being on, but I can do that and see if we just want to keep it on always.

Directory.Build.props

eng/Build.props

eng/Signing.props

src/finalizer/finalizer.nativeproj

src/redist/packaging/osx/resources/welcome.html.lci

nagilson

Whoops, this was supposed to be on the other PR, let me dismiss this... 😄

…Provider.dll again.

ViktorHofer

Approving the src/SourceBuild change

…ller-merge-update # Conflicts: # .vsts-ci.yml # eng/Publishing.props # eng/Versions.props # eng/build.yml # global.json # src/redist/targets/BundledTemplates.targets # src/redist/targets/GenerateBundledVersions.targets # src/redist/targets/Versions.targets

# Conflicts: # eng/Version.Details.xml # eng/Versions.props # global.json

MiYanni added 15 commits November 10, 2023 17:07

Remove no longer applicable workaround in run-build.ps1. Applied expr…

c9ab438

…ession bodied properties to solution.

Applied expression bodies for methods to solution.

67cbc0d

Cleanup extra qualification. Cleanup redundant casting. Cleanup using…

c42fb4c

…s. Set var suggestion to none.

Lots of minor cleanup and simplification.

b25079d

Minor simplification and cleanup. The record I created didn't work fo…

32c3e4e

…r the 4.7.2 build, so changed it to class.

Minor formatting cleanup.

b5323ac

Minor formatting cleanup.

93399cf

Added some comments to Version.Details.xml for packages that may not …

3f7756f

…be necessary. Manual formatting cleanup for targets in the redist project.

Moved packaging up a directory into redist from targets.

d35882b

Moved contents of both clisdk folders from osx/windows up a directory.

cdf09f8

Update pathing for the moving of packaging and clisdk folders/files.

a6fd8ca

Added Valleysoft.DockerCredsProvider.dll to be ignored for signing.

47cb621

Simplify the DownloadPackage.csproj by using the NoTargets SDK.

7e94dea

Removed LayoutTool.csproj. Reused DownloadPackage.csproj and merged t…

049f965

…he target from LayoutTool.csproj directly into where it was used in BundledDotnetTools.targets. Added comments to DownloadPackage.csproj.

Move SdkResolver.csproj and VSTemplateLocator.csproj into projects un…

396af3e

…der redist. Added more comments to DownloadPackage.csproj.

MiYanni changed the title ~~[WIP] Installer (Merge to SDK) - Changeset~~ [NO MERGE] Installer (Merge to SDK) - Changeset Dec 13, 2023

MiYanni added the DO NOT MERGE label Dec 13, 2023

MiYanni added 13 commits December 12, 2023 18:19

Move DownloadPackage.csproj to the projects directory for redist. Cle…

a4235dc

…an up formatting in SdkTests.csproj. Removed the empty <TargetPath></TargetPath> from SdkTests.csproj to allow loading in VS.

Update file comment about usage in DownloadPackage.csproj.

d9cc884

Use Microsoft.Build.NoTargets for the SdkResolver.csproj and VSTempla…

ed59078

…teLocator.csproj projects. Added the projects back to the SLN so they build using the proper configuration (debug/release).

Instead of making it complicated, I simply made these 2 packaging pro…

bec677e

…jects only ever build with the default configuration (Debug) since it is irrelevant for packaging. Updated the paths used when creating the MSIs, which uses the output of these to create NuGet packages.

Minor formatting fixes. TODO comments for tracking versions/packages.

756e6e0

Version alignment with the SDK branch.

8b09739

No longer needed these TODOs as FSharp version is aligned.

7309797

Minor formatting cleanup.

046c159

Version alignment with SDK repo branch.

0ac3fc7

General formatting and cleanup. Removed GivenThatWeWantToRunCrossgen …

23d9262

…from SdkTests as it hasn't existed in years...?

Fix missing single quote in SdkTests.csproj.

8e86e03

Fix bugged BundledDotnetTools.targets execution (wasn't processing th…

c0806dd

…e item group properly).

MiYanni commented Feb 28, 2024

View reviewed changes

src/redist/targets/GenerateMSIs.targets Outdated Show resolved Hide resolved

MiYanni commented Feb 28, 2024

View reviewed changes

src/redist/targets/LayoutTool.csproj Outdated

Copy link

Member Author

MiYanni Feb 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in favor of DownloadPackage.csproj instead.

MiYanni commented Feb 28, 2024

View reviewed changes

MiYanni added 3 commits February 28, 2024 12:12

Added Valleysoft.DockerCredsProvider.dll exclusion back as it is nece…

5a9e492

…ssary. Updated or removed TODO comments. Fixed some tabbing from merging. Fixed a compilation bug if this code is ever built against 4.5.1(????)

Adjusted the way in which Platform is set for finalizer. Now, it is s…

35bfedb

…et prior to calling the Arcade SDK, so the OutputPath properly includes the Architecture folder.

Make sure Platform is TreatAsLocalProperty so it can be set properly.

f0325a2

joeloff reviewed Mar 7, 2024

View reviewed changes

joeloff approved these changes Mar 7, 2024

View reviewed changes

akoeplinger approved these changes Mar 8, 2024

View reviewed changes

nagilson approved these changes Mar 13, 2024

View reviewed changes

nagilson self-requested a review March 13, 2024 22:34

Addressing PR feedback. Trying the signing for Valleysoft.DockerCreds…

d94e51d

…Provider.dll again.

MiYanni requested a review from a team as a code owner April 2, 2024 22:47

ViktorHofer approved these changes Apr 3, 2024

View reviewed changes

MiYanni added 2 commits April 5, 2024 11:17

Merge branch 'main' into feature/installer-merge

378b683

# Conflicts: # eng/Version.Details.xml # eng/Versions.props # global.json

MiYanni enabled auto-merge April 5, 2024 20:59

akoeplinger added 2 commits April 6, 2024 09:41

Temporarily comment astextplain diff in .gitattributes

712b8fe

Use binary instead of astextplain

7295cdf

MiYanni merged commit 899cd0f into main Apr 6, 2024

MiYanni deleted the feature/installer-merge branch April 6, 2024 18:13

MiYanni mentioned this pull request Apr 9, 2024

Merge dotnet/Installer src and test folder histories dotnet/sdk#40084

Merged

		@@ -183,14 +179,11 @@ private string BuildCmdParameters(ConfigJson configJson, string package_version)

		// Map all the payload directories as they need to install on the system
		if (configJson.Install_Root != null)

Installer updates (Merge to SDK Changeset) #17959

Installer updates (Merge to SDK Changeset) #17959

Uh oh!

Conversation

MiYanni commented Dec 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

High-Level Changes

Things of Note

Finalizer Changes Details

Test Builds

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MiYanni Apr 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nagilson left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ViktorHofer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MiYanni commented Dec 13, 2023 •

edited

Loading

MiYanni Apr 2, 2024 •

edited

Loading

nagilson left a comment •

edited

Loading