-
Notifications
You must be signed in to change notification settings - Fork 73
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
fix: do not rewrite absolute paths to avoid long paths #74
Conversation
This is an alternative to f2c7618.
Tests passed upstream with the changes applied to |
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.
Does this mean absolute paths still run the risk of breaking the 260 boundary?
Since this gets us green, it's probably better than what we have now, so if you want to cut a gyp-next release, someone can put up a vendor PR to node-gyp and I can cut a release in short order. I'm off for the night but if those things can be done before tomorrow I can move fairly quickly on a release with just this change.
No, because skipping this part for absolute paths means that we do not rename them at all, and in this case MSVC just puts the file in the output directory instead of a subdirectory. |
Are we going to do an update of gyp-next and node-gyp today or should we float this on npm? |
A change to "correctly rename object files for absolute paths" caused a bug in windows. A fix has already landed upstream in gyp-next, but has not yet made it's way to a release of gyp-next or to node-gyp. Let's float the change until it is fixed upstream. Refs: nodejs/gyp-next#74
A change to "correctly rename object files for absolute paths" caused a bug in windows. A fix has already landed upstream in gyp-next, but has not yet made it's way to a release of gyp-next or to node-gyp. Let's float the change until it is fixed upstream. Refs: nodejs/gyp-next#74 PR-URL: #1974 Credit: @MylesBorins Close: #1974 Reviewed-by: @ruyadorno
This is an alternative to f2c7618.
@MylesBorins @rvagg