-
Notifications
You must be signed in to change notification settings - Fork 63
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
Added Support for Building on Windows #454
Conversation
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.
Thanks for the PR! I have a few minor questions regarding these changes before we get this in.
|
||
# Transpile individual files. This is useful if another module, | ||
# e.g. cycledash, wants to require('pileup'). | ||
# The dist/test files are required for code coverage | ||
mkdir -p dist/test/{data,source,viz} | ||
babel src --retain-lines --ignore src/lib --out-dir dist | ||
node_modules/.bin/babel src --retain-lines --ignore src/lib --out-dir dist |
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.
This absolute path will break things if people have some of these tools already installed globally, right?
And just to clarify things: this script should be run through npm
via npm run build
instead of sh scripts/build.sh
. The former should adjust the PATH
on the fly to make sure that all installed packages are in the PATH
. Can you confirm that this doesn't happen on Windows/cgi-bin environment?
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.
The path will not work for those who have these things installed globally, that is correct.
On Windows it should not be run through NPM due to the ./ syntax used. That syntax is not supported by Cygwin. Instead of rewriting all the NPM scripts to play nice with Cygwin, I thought it was easier and less error prone to simply change build.sh.
As for the PATH adjustments, I'm not sure what you mean.
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.
ah, gotcha! Didn't realize that the npm run build
was failing due to the way scripts
are defined in package.json
.
I tried googling a solution for this, realized that this is a common issue, and it is NPM-related rather than pileup itself. Furthermore, NPM people officially do not support cygwin
and recommend against using it: https://github.com/npm/npm#installing-on-cygwin
Although it would be great to support cygwin, I don't think we should make strong assumptions about people's setup and just do the things in the NPM way as much as possible. I didn't know about the cygwin/npm
interaction was that problematic and I think it is not up to us to start adjusting things for a platform that is not supported.
I really appreciate the PR and your contribution, but I don't think we should merge this into pileup.
@@ -1,12 +1,13 @@ | |||
#!/bin/bash | |||
(set -o igncr) 2>/dev/null && set -o igncr; # For Cygwin on Windows compatibility |
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.
can you explain what this does a bit more and maybe put a pointer to the comments? I am asking this because if we hit an issue because of this setting, we then can go back and check the details in the future.
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.
igncr is a Cygwin only setting which causes Cygwin bash to recognize \n\r as line endings as opposed to recognising the purely Linux \n as a line ending and then \r as an error.
This line attempts to set the option, suppresses any error messages. If it is not set successively then abort. If there is no error, then the option is set for real.
The snippet in question is from Eric Blake on the Cygwin mailing list from a few years back https://cygwin.com/ml/cygwin-announce/2010-08/msg00015.html this solution particularly seemed like the most painless to implement.
Turns out NPM officially doesn't support cygwin: https://github.com/npm/npm#installing-on-cygwin hence we shouldn't try to work around cygwin-specific problems in this package. Closing the PR. |
@armish fair enough. I appreciate the speedy replies and discussion! |
Some minor changes to the build toolchain to support building of PileupJS on Windows. Confirmed works on Windows.
This change is