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: Address issues encountered while building on Windows+Cygwin environments #1817

Closed
wants to merge 1 commit into from
Closed

fix: Address issues encountered while building on Windows+Cygwin environments #1817

wants to merge 1 commit into from

Conversation

joquijada
Copy link

@joquijada joquijada commented Jul 10, 2019 •
edited
Loading

Address problems described here that were encountered during npm rebuild of modules that depend on node-gyp to compile C/C++ code.

Fixes: #1782
Refs: https://github.com/joquijada/windows-node-js-node-gyp

Checklist
  • npm install && npm test passes
  • tests are included
  • documentation is changed or added (N/A)
  • commit message follows commit guidelines
Description of change

cclauss
cclauss requested changes Jul 10, 2019
View reviewed changes
gyp/gyp_main.py Outdated Show resolved Hide resolved
gyp/pylib/gyp/common.py Outdated Show resolved Hide resolved
cclauss
cclauss reviewed Jul 10, 2019
View reviewed changes
gyp/gyp_main.py Outdated Show resolved Hide resolved
cclauss
cclauss reviewed Jul 10, 2019
View reviewed changes
gyp/pylib/gyp/common.py Outdated Show resolved Hide resolved
cclauss
cclauss reviewed Jul 10, 2019
View reviewed changes
gyp/gyp_main.py Outdated Show resolved Hide resolved
cclauss
cclauss reviewed Jul 10, 2019
View reviewed changes
gyp/pylib/gyp/common.py Outdated Show resolved Hide resolved
joquijada pushed a commit to joquijada/node-gyp that referenced this pull request Jul 10, 2019
fix: New style exception
24d0be3 
Use new style exceptions so that it works in both Python 2 and 3

Refs: nodejs#1817 (comment)
joquijada pushed a commit to joquijada/node-gyp that referenced this pull request Jul 10, 2019
fix: New style exception
01495e9 
Use new style exceptions so that it works in both Python 2 and 3. Doing the
same as previous commit, but in a different file.

Refs: nodejs#1817 (comment)
cclauss
cclauss reviewed Jul 10, 2019
View reviewed changes
gyp/pylib/gyp/common.py Outdated Show resolved Hide resolved
@joquijada joquijada changed the title WIP: fix: Addresses issues encountered while building on WIndows+Cygwim environments fix: Addresses issues encountered while building on WIndows+Cygwim environments Jul 10, 2019
@joquijada joquijada changed the title fix: Addresses issues encountered while building on WIndows+Cygwim environments fix: Addresses issues encountered while building on WIndows+Cygwin environments Jul 10, 2019
@joquijada joquijada changed the title fix: Addresses issues encountered while building on WIndows+Cygwin environments fix: Addresses issues encountered while building on Windows+Cygwin environments Jul 10, 2019
cclauss
cclauss approved these changes Jul 10, 2019
View reviewed changes
Copy link
Contributor

@cclauss cclauss left a comment

Choose a reason for hiding this comment

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

Nice work.

rvagg
rvagg reviewed Jul 15, 2019
View reviewed changes
lib/configure.js Outdated

// Do this to keep Cygwin environments happy, else the unescaped '\' gets eaten up,
// resulting in bad paths, Ex c:parentFolderfolderanotherFolder instead of c:\parentFolder\folder\anotherFolder
nodeLibFile = nodeLibFile.replace(/\\/g, '\\\\')
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is a windows-only branch so to be safe maybe this should only be invoked if windows

Copy link
Contributor

Choose a reason for hiding this comment

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

Do posix paths contain backslashes?

Copy link
Member

Choose a reason for hiding this comment

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

$ touch 'foo\\bar'
$ ls -al foo\\\\bar
-rw-r--r-- 1 rvagg staff 0 Jul 15 16:56 'foo\\bar'

Copy link
Member

Choose a reason for hiding this comment

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

This still needs to be addressed @joquijada. I think can you wrap this in a conditional for Cygwin or at least Windows

Copy link
Author

Choose a reason for hiding this comment

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

@rvagg Done, wrapped in a Windows conditional.

rvagg
rvagg reviewed Jul 15, 2019
View reviewed changes
gyp/gyp_main.py Show resolved Hide resolved
@cclauss cclauss mentioned this pull request Jul 15, 2019
Open
@joquijada
Copy link
Author

I wasn't sure if I should/can write tests for this.

@joquijada joquijada changed the title fix: Addresses issues encountered while building on Windows+Cygwin environments fix: Address issues encountered while building on Windows+Cygwin environments Jul 23, 2019
@joquijada joquijada force-pushed the 1782-node-gyp-not-found-cygwin-windows branch from ba0e3b5 to 908b9b0 Compare July 23, 2019 01:55
@joquijada
Copy link
Author

joquijada commented Jul 26, 2019 •
edited
Loading

FYI, squashed the commits into 1 via git rebase -i upstream/master

@refack
Copy link
Contributor

refack commented Jul 26, 2019

Just an FYI, since Cygwin is not officially supported it has been removed from GYP3, so expect #1845 to regress things 🤷‍♂

IMHO Cygwin support could be more easily maintained for node-gyp@4 and/or 3. Probably with a little more tweaking also for 5 via the make-linux GYP3 output generator.

@joquijada joquijada force-pushed the 1782-node-gyp-not-found-cygwin-windows branch from 908b9b0 to 8770043 Compare July 27, 2019 12:34
@rvagg
Copy link
Member

rvagg commented Jul 29, 2019

@refack would there be any objections to adding it back in to GYP3 if someone wants to put in the work? I assume it might not be too hard to figure out the changes that impact support and improve them for Cygwin.

rvagg pushed a commit that referenced this pull request Aug 1, 2019
@rvagg
gyp: improve Windows+Cygwin compatibility
c7f1bca 
Fixes: #1782
PR-URL: #1817
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Rod Vagg <r@va.gg>
@rvagg
Copy link
Member

rvagg commented Aug 1, 2019

Landed in c7f1bca after some whitespace changes and commit message edits.

@joquijada the caveat above about the coming GYP upgrade is important because these changes aren't necessarily going to be carried over and we don't Cygwin in any of our testing paths. You may need to come back and help get us sorted out but https://github.com/refack/GYP will probably be where GYP changes need to go.

@rvagg rvagg closed this Aug 1, 2019
@joquijada
Copy link
Author

@rvagg Will check it out

@rvagg rvagg mentioned this pull request Sep 26, 2019
Closed
rvagg pushed a commit that referenced this pull request Sep 26, 2019
@rvagg
gyp: improve Windows+Cygwin compatibility
5553cd9 
Fixes: #1782
PR-URL: #1817
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Rod Vagg <r@va.gg>
@rvagg rvagg mentioned this pull request Sep 26, 2019
Closed
@joquijada joquijada deleted the 1782-node-gyp-not-found-cygwin-windows branch October 22, 2019 21:38
@targos targos mentioned this pull request Nov 27, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardlau richardlau richardlau left review comments

@cclauss cclauss cclauss approved these changes

@rvagg rvagg rvagg left review comments

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

Successfully merging this pull request may close these issues.

ImportError: No module named gyp on Cygwin/Windows
5 participants
@joquijada @refack @rvagg @cclauss @richardlau