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

gyp: show descriptive Windows SDK detection error #14597

Closed
wants to merge 1 commit into from
Closed

gyp: show descriptive Windows SDK detection error #14597

wants to merge 1 commit into from

Conversation

jaimecbernardo
Copy link
Contributor

When building with Visual Studio 2017, gyp may fail with a non-descriptive message if Windows has stale registry keys for a version of Windows SDK that was previously uninstalled.

This commit adds a specific warning message when the directory for a detected SDK version doesn't exist and adds some fixes to avoid Python crashes that were blocking the detection of other SDK versions:

  • Only try to run listdir on a path if it exists and is a dir.
  • Avoid accessing names[0] if it has no elements.
  • Use %s instead of %o to print compatible_sdks (to avoid TypeError, since %o is the octal number format specifier in Python and %s can be used as a generic format specifier for objects).

Fixes: #14103

/cc @nodejs/build

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

win, build, tools, gyp

@jaimecbernardo
gyp: show descriptive Windows SDK detection error
4715d00 
When building with Visual Studio 2017, gyp may fail with a
non-descriptive message if Windows has stale registry keys for a
version of Windows SDK that was previously uninstalled.

This commit adds a specific warning message when the directory for
a detected SDK version doesn't exist and adds some fixes to avoid
Python crashes that were blocking the detection of other SDK
versions:

- Only try to run listdir on a path if it exists and is a dir.

- Avoid accessing names[0] if it has no elements.

- Use %s instead of %o to print compatible_sdks (to avoid TypeError,
since %o is the octal number format specifier in Python and %s can be
used as a generic format specifier for objects).

Fixes: nodejs#14103
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory. labels Aug 2, 2017
@jaimecbernardo
Copy link
Contributor Author

/cc @bnoordhuis /cc @refack PTAL

refack
refack reviewed Aug 2, 2017
View reviewed changes
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

Looks good

@refack
Copy link
Contributor

refack commented Aug 2, 2017 •
edited
Loading

@jaimecbernardo this looks good, would you mind submitting it upstream: https://gyp.gsrc.io/docs/Hacking.md

P.S. If you do add me as a reviewer

@refack refack added the python PRs and issues that require attention from people who are familiar with Python. label Aug 2, 2017
@jaimecbernardo
Copy link
Contributor Author

@refack Will submit it upstream, thanks!

@jaimecbernardo
Copy link
Contributor Author

I've submitted this PR upstream. Review link: https://chromium-review.googlesource.com/c/602133

@jaimecbernardo
Copy link
Contributor Author

This PR has landed upstream in https://chromium.googlesource.com/external/gyp/+/324dd166b7c0b39d513026fa52d6280ac6d56770

@refack
Copy link
Contributor

refack commented Aug 9, 2017

@jaimecbernardo thanks, I'll update #12632 (or open a new PR, since that one seems stalled).

@jaimecbernardo
Copy link
Contributor Author

@refack I guess it makes more sense to update the current PR, if you can :) Thank you.

@jaimecbernardo
Copy link
Contributor Author

@refack Actually, that PR is for ninja, right? This commit doesn't seem to fall under that umbrella.

@refack
Copy link
Contributor

refack commented Aug 9, 2017

@refack Actually, that PR is for ninja, right? This commit doesn't seem to fall under that umbrella.

#12632 started as a GYP bump to enable ninja on windows, but now it's just a GYP bump.
I'll close it and open a new PR with a new title, bumping GYP upto and including 324dd166b7c0b39d513026fa52d6280ac6d56770 maybe it'll be approved faster 🤷‍♂️

@refack
Copy link
Contributor

refack commented Aug 9, 2017

So I'm closing this in favor of a new GYP bump PR - #14718

@refack refack closed this Aug 9, 2017
@refack
Copy link
Contributor

refack commented Aug 9, 2017

@jaimecbernardo Thanks for the contribution and the "bureaucratic" dance 🥇

@jaimecbernardo
Copy link
Contributor Author

@refack Thanks for helping me along the way ;)

duqingnian pushed a commit to duqingnian/gyp that referenced this pull request Apr 26, 2024
Update patch set 1
23b91cd 
Patch Set 1:

Hi,

This change comes originally from nodejs/node#14597

I've added as reviewers those accounts that seem to have committed/reviewed recent changes for Windows in gyp.
Please let me know if I should add another reviewer I might have missed.

Thanks, in advance.

Patch-set: 1
Reviewer: Gerrit User 1188132 <1188132@3ce6091f-6c88-37e8-8c75-72f92ae8dfba>
Reviewer: Gerrit User 1174099 <1174099@3ce6091f-6c88-37e8-8c75-72f92ae8dfba>
Reviewer: Gerrit User 1003232 <1003232@3ce6091f-6c88-37e8-8c75-72f92ae8dfba>
Reviewer: Gerrit User 1226816 <1226816@3ce6091f-6c88-37e8-8c75-72f92ae8dfba>
Work-in-progress: false
duqingnian pushed a commit to duqingnian/gyp that referenced this pull request Apr 26, 2024
@jaimecbernardo
gyp: show descriptive Windows SDK detection error
66db260 
When building with Visual Studio 2017, gyp may fail with a
non-descriptive message if Windows has stale registry keys for a
version of Windows SDK that was previously uninstalled.

This commit adds a specific warning message when the directory for
a detected SDK version doesn't exist and adds some Fixes to avoid
Python crashes that were blocking the detection of other SDK
versions:

- Only try to run listdir on a path if it exists and is a dir.

- Avoid accessing names[0] if it has no elements.

- Use %s instead of %o to print compatible_sdks (to avoid TypeError,
since %o is the octal number format specifier in Python and %s can be
used as a generic format specifier for objects).

Refs: nodejs/node#14597
Bug: nodejs/node#14103
Change-Id: Ifd50fe239f65b7b4a2d69c1c02038bada03066cb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@refack refack refack left review comments

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. python PRs and issues that require attention from people who are familiar with Python. tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

vcbuild fails with non-descriptive error when the registry has stale keys
3 participants
@jaimecbernardo @refack @nodejs-github-bot