Skip to content

Fix path to TarTool in Sign.proj #15299

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Dec 4, 2024
Merged

Fix path to TarTool in Sign.proj #15299

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e0c64a2
Fix path to TarTool in Sign.proj
ViktorHofer Dec 4, 2024
85d7d32
Fix TarToolPath to correct directory path
ViktorHofer Dec 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/Microsoft.DotNet.Arcade.Sdk/tools/Sign.proj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@
<_DotNetCorePath>$(DotNetTool)</_DotNetCorePath>
</PropertyGroup>

<!-- Keep TarToolPath TFM in sync with TarTool project TFM. -->
Copy link
Member

@akoeplinger akoeplinger Dec 4, 2024

Choose a reason for hiding this comment

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

ok as a quick fix but nobody will remember to update this place in the future.

What about doing something like:

<ItemGroup>
  <_TarToolPath Include="$(NuGetPackageRoot)microsoft.dotnet.tar\$(MicrosoftDotNetSignToolVersion)\tools\net*\any\Microsoft.Dotnet.Tar.dll" />
</ItemGroup>

Copy link
Member

@fhnaseer fhnaseer Dec 4, 2024

Choose a reason for hiding this comment

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

I would suggest logging an error/warning if tool is not found. Right now, it fails silently.

arcade/src/Microsoft.DotNet.SignTool/src/ZipData.cs

Line 249 in 7fff497

private static bool RunTarProcess(string srcPath, string dstPath, string tarToolPath)

Copy link
Member Author

@ViktorHofer ViktorHofer Dec 4, 2024

Choose a reason for hiding this comment

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

I think that would include the net472 one as well. Arcade continues to have a few hardcoded TFMs, i.e. in #15221.

This is the only PackAsTool component that is directly referenced in Arcade. @ellahathaway / @mmitche would it make sense for SignTool to redistribute the TarTool and optionally reference it as a classlib?

Copy link
Member Author

@ViktorHofer ViktorHofer Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Alternatively, it should be possible to save the TFM in Arcade's generated DefaultVersions.Generated.props file and then read from that here.

Copy link
Member

@mmitche mmitche Dec 4, 2024

Choose a reason for hiding this comment

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

I have a fix for the exit non-zero case in a local branch that should be up soon. That said, I don't think TarTool is actually in use. On framework it just bails:

https://github.com/dotnet/arcade/blob/main/src/Microsoft.DotNet.Tar/Program.cs#L4-L9

And in signtool, we only use tartool on framework:

https://github.com/dotnet/arcade/blob/main/src/Microsoft.DotNet.SignTool/src/ZipData.cs#L248-L326

I will problaby rip it out.

Copy link
Member

@akoeplinger akoeplinger Dec 4, 2024

Choose a reason for hiding this comment

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

I think that would include the net472 one as well.

For the record it wouldn't since we only pack it as a tool in netcore.

<Microsoft.DotNet.SignTool.SignToolTask
DryRun="$(_DryRun)"
TestSign="$(_TestSign)"
Expand All @@ -97,7 +98,7 @@
SNBinaryPath="$(SNBinaryPath)"
MicroBuildCorePath="$(NuGetPackageRoot)microbuild.core\$(MicroBuildCoreVersion)"
WixToolsPath="$(WixInstallPath)"
TarToolPath="$(NuGetPackageRoot)microsoft.dotnet.tar\$(MicrosoftDotNetSignToolVersion)\tools\net\any\Microsoft.Dotnet.Tar.dll"
TarToolPath="$(NuGetPackageRoot)microsoft.dotnet.tar\$(MicrosoftDotNetSignToolVersion)\tools\net9.0\any\Microsoft.Dotnet.Tar.dll"
RepackParallelism="$(SignToolRepackParallelism)"
MaximumParallelFileSize="$(SignToolRepackMaximumParallelFileSize)" />
</Target>
Expand Down
Loading