Skip to content
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

Jump to bottom

Fix #1186 Fixing signtool.exe path for new windows kits #1188

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from
Open

Fix #1186 Fixing signtool.exe path for new windows kits #1188

wants to merge 9 commits into from

Conversation

gustavocalheiros
Copy link

@gustavocalheiros gustavocalheiros commented May 31, 2023 •
edited
Loading

I look inside the path of the latest Windows Kit major version (in this case "10") and check in all the minor version folders (for example "10.0.22621.0", "10.0.16299.0"), and get the last one that contains the file "signtool.exe".

if nothing is found, I fallback on the previous code.

I confirm that the pull-request:

  • Follows the contribution guidelines
  • Is based on my own work
  • Is in compliance with my employer

@gustavocalheiros gustavocalheiros changed the title Fix https://github.com/nuke-build/nuke/issues/1186 Fixing signtool.ex… fixes #1186 Fixing signtool.exe path for new windows kits May 31, 2023
@gustavocalheiros gustavocalheiros changed the title fixes #1186 Fixing signtool.exe path for new windows kits Fix #1186 Fixing signtool.exe path for new windows kits May 31, 2023
@gustavocalheiros
Copy link
Author

Fix #1186

@matkoch
Copy link
Member

matkoch commented Jun 13, 2023

Could you unify the code to only make use of GlobFiles ?

@matkoch matkoch linked an issue Jun 27, 2023 that may be closed by this pull request
"SignToolTasks.SignToolPath" can't find the correct path for signtool.exe #1186
Open
@gustavocalheiros
Copy link
Author

yep, will do it in the next days.

@gustavocalheiros
Copy link
Author

gustavocalheiros commented Jun 28, 2023 •
edited
Loading

@matkoch do you mind if we drop the support to old versions of the Windows Kit (I mean support only 10.*) ?

PS: Windows Kit 10 is the current version of the SDK for windows 10 and 11.

@matkoch
Copy link
Member

matkoch commented Jun 28, 2023

fine i guess

@gustavocalheiros
Copy link
Author

@matkoch done

@matkoch matkoch force-pushed the develop branch 3 times, most recently from 0686f32 to d6aab06 Compare August 21, 2023 17:58
@gustavocalheiros
Copy link
Author

hello @matkoch, any chance this PR will be merged soon?

@matkoch matkoch force-pushed the develop branch 5 times, most recently from 0231425 to d806295 Compare December 15, 2023 23:12
@matkoch matkoch force-pushed the develop branch 3 times, most recently from 7066bbe to 985de83 Compare January 17, 2024 17:25
@matkoch matkoch force-pushed the develop branch 4 times, most recently from 0719711 to 6fe1e3b Compare March 20, 2024 01:24
MichaeIDietrich
MichaeIDietrich reviewed Mar 31, 2024
View reviewed changes
source/Nuke.Common/Tools/SignTool/SignToolSettings.cs Outdated
.FirstOrDefault();
var windowsKitsRootDirectory = programDirectory / "Windows Kits" / windowsKitLastVersion;

var signToolPath = windowsKitsRootDirectory.GlobFiles($"bin/{windowsKitLastVersion}.*/{platformIdentifier}/signtool.exe").LastOrDefault();
Copy link

Choose a reason for hiding this comment

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

Shouldn't we order by version number first before using LastOrDefault?

Copy link
Author

Choose a reason for hiding this comment

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

the order returned is naturally the alphabetical order, meaning version 10.0.22 will be after version 10.0.17.

Copy link

Choose a reason for hiding this comment

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

Thanks for the info, I did not know it's already ordered. 👍

@MichaeIDietrich
Copy link

What do you think about adding support for using the signtool.exe from these two NuGets?
https://www.nuget.org/packages/Microsoft.Windows.SDK.BuildTools
https://www.nuget.org/packages/signtool

@gustavocalheiros
Copy link
Author

What do you think about adding support for using the signtool.exe from these two NuGets? https://www.nuget.org/packages/Microsoft.Windows.SDK.BuildTools https://www.nuget.org/packages/signtool

@MichaeIDietrich Do you think it is possible to the user to run nuke-build having no SDK kit installed?
if yes, I'm ok to add support only to the 1st one (official microsoft package). What do you think?

@gpailler
Copy link
Contributor

gpailler commented Apr 4, 2024

Hello @gustavocalheiros, I would say yes. The Microsoft SDK is not a prerequisite for Nuke Build

@MichaeIDietrich
Copy link

Yes, the Windows SDK is not needed for Nuke to run. Only a matching .NET SDK is needed to run Nuke.

@gustavocalheiros
Copy link
Author

@MichaeIDietrich Pushed support for Microsoft.Windows.SDK.BuildTools

gustavocalheiros
gustavocalheiros commented Apr 5, 2024
View reviewed changes
source/Nuke.Common/Tools/SignTool/SignToolSettings.cs
var nugetPath = SettingsUtility.GetGlobalPackagesFolder(Settings.LoadDefaultSettings(null));

signToolPath = AbsolutePath.Create(nugetPath)
.GlobFiles($"{microsoftBuildToolsNugetPackage}/{windowsKitVersionWildcard}/bin/{windowsKitVersionWildcard}/{platformIdentifier}/{signtoolExe}")
Copy link
Author

@gustavocalheiros gustavocalheiros Apr 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

path structure for nuget packages is different, for example :
C:\Users\MyUser\ .nuget\packages\microsoft.windows.sdk.buildtools\10.0.20348\bin\10.0.20348\x64\signtool.exe

@gpailler
Copy link
Contributor

gpailler commented Apr 5, 2024

@MichaeIDietrich @matkoch
Do you think we should align with the approach taken by other packages, such as XUnit and Codecov, by updating SignTool.json with:

  "nugetPackageId": "Microsoft.Windows.SDK.BuildTools",
  "customExecutable": true,

and implementing the GetProcessToolPath() method?
This would be a breaking change, but it seems to be more in line with the conventions used by other tools.

@matkoch matkoch force-pushed the develop branch 3 times, most recently from 6d02194 to 4bfee84 Compare June 13, 2024 21:33
@matkoch matkoch force-pushed the develop branch 4 times, most recently from 85998da to 01b2b6e Compare July 9, 2024 00:20
@matkoch matkoch force-pushed the develop branch 3 times, most recently from e652c9a to 6ed9e4d Compare July 14, 2024 18:27
@matkoch matkoch force-pushed the develop branch 2 times, most recently from df620ac to 482f5ac Compare November 26, 2024 01:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MichaeIDietrich MichaeIDietrich Awaiting requested review from MichaeIDietrich

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

"SignToolTasks.SignToolPath" can't find the correct path for signtool.exe
4 participants
@gustavocalheiros @matkoch @MichaeIDietrich @gpailler