-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
gyp: fix regex to match multi-digit versions #1455
Conversation
This fixes the regular expression matching in `xcode_emulation` to also handle version numbers with multiple-digit major versions which would otherwise break under use of XCode 10 Fixes: #1454
There currently aren't any tests for this particular part of the code – should I add some? If so, I think I could do with some guidance on that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks.
As to testing, there's no direct way to test this code since it lives inside gyp, not node-gyp, which has its own external test suite.
Oh, I see, of course – not sure where my head was. I guess it would be preferable to land this upstream (if it hasn't been patched there yet)? |
Ideally, yes, but Google has effectively abandoned gyp so we're in this awkward situation where we're the de facto (but not de jure) upstream. |
I've confirmed that this change along unbreaks things on macOS High Sierra with CLI tools. Can we expedite landing and publishing? |
/ping @nodejs/node-gyp |
Sweet, thank you for the swift response! |
still an issue |
Hm, I'm not sure this has fully addressed the issue. On my system, |
@nornagon Would it be possible to add a new Travis CI run which mimics your Mac setup? |
Checklist
npm install && npm test
passesDescription of change
This fixes the regular expression matching in
xcode_emulation
to also handle version numbers with multiple-digit major versions
which would otherwise break under use of XCode 10
Fixes: #1454