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.

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

win: more robust parsing of SDK version #1198

Merged
merged 1 commit into from
May 17, 2017

Conversation

refack
Copy link
Contributor

@refack refack commented May 13, 2017

Fixes: #1179

Personally I prefer #1178, but this is smaller.

@refack refack mentioned this pull request May 13, 2017
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM with a question.

Win10SDKVer = Math.Max(Win10SDKVer, UInt32.Parse(id.Substring(Win10SDKPrefix.Length)));
else if (id == "Microsoft.VisualStudio.Component.Windows81SDK")
else if (id.StartsWith(Win10SDKPrefix)) {
string[] parts = id.ToString().Substring(Win10SDKPrefix.Length).Split('.');
Copy link
Member

Choose a reason for hiding this comment

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

Superfluous ToString() call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep.
Gone.

else if (id == "Microsoft.VisualStudio.Component.Windows81SDK")
else if (id.StartsWith(Win10SDKPrefix)) {
string[] parts = id.Substring(Win10SDKPrefix.Length).Split('.');
Win10SDKVer = Math.Max(Win10SDKVer, UInt32.Parse(parts[0]));
Copy link
Member

Choose a reason for hiding this comment

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

Only the Desktop C++ SDK is good, according to https://docs.microsoft.com/en-us/visualstudio/install/workload-component-id-vs-community it has .Desktop appended. What about:

-                    Win10SDKVer = Math.Max(Win10SDKVer, UInt32.Parse(parts[0]));
+                    if (parts.Length == 1 || (parts.Length == 2 && parts[1] == "Desktop"))
+                        Win10SDKVer = Math.Max(Win10SDKVer, UInt32.Parse(parts[0]));
+                    else
+                        continue;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack, with variation

else if (id == "Microsoft.VisualStudio.Component.Windows81SDK")
else if (id.StartsWith(Win10SDKPrefix)) {
string[] parts = id.Substring(Win10SDKPrefix.Length).Split('.');
if (parts.Length == 2 && parts[1] != "Desktop")
Copy link
Member

Choose a reason for hiding this comment

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

There is already a Microsoft.VisualStudio.Component.Windows10SDK.15063.UWP.Native that this does not catch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👎 for Microsoft.
Enhanced the guard.

Copy link
Member

@joaocgreis joaocgreis left a comment

Choose a reason for hiding this comment

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

LGTM

@joaocgreis
Copy link
Member

CI: https://ci.nodejs.org/view/All/job/nodegyp-test-pull-request/13/

@refack can you land here or should I? (we should still wait for the 72h)

@refack
Copy link
Contributor Author

refack commented May 15, 2017

@joaocgreis I don't have write here (yet?), after we land we need to ping one of the package owners to package a patch version.

@joaocgreis
Copy link
Member

v0.12 failed, here is another run: https://ci.nodejs.org/job/nodegyp-test-commit/113/ . This one passed.

PR-URL: nodejs#1198
Fixes: nodejs#1179
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: João Reis <reis@janeasystems.com>
@refack refack merged commit bad903a into nodejs:master May 17, 2017
@refack
Copy link
Contributor Author

refack commented May 17, 2017

Landed in bad903a

@refack
Copy link
Contributor Author

refack commented May 17, 2017

@refack
Copy link
Contributor Author

refack commented May 17, 2017

This should go into v3.6.2 fixes #1179 and #1144 (comment)
/cc @bnoordhuis @rvagg

This was referenced May 22, 2017
@refack
Copy link
Contributor Author

refack commented May 28, 2017

/cc @rvagg @bnoordhuis @joaocgreis @nodejs/node-gyp
IMHO we need to release another patch version for this (and try to push it into npm@5)
Fixes: #1179
Ref: #1056 (comment)
Ref: #1144 (comment)
Ref: #1205

@rvagg
Copy link
Member

rvagg commented Jun 1, 2017

👍 done, v3.6.2, thanks @refack

@refack refack deleted the fix-1179-parsing branch June 1, 2017 22:29
@refack
Copy link
Contributor Author

refack commented Jun 1, 2017

👍 done, v3.6.2, thanks @refack

No, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants