fix(build): Support running npm scripts under Windows#1251
Merged
Conversation
TimoStaudinger
commented
Jan 25, 2019
| "scripts": { | ||
| "prebuild": "node ./build/generateIcons.js", | ||
| "build": "yarn build:babel; yarn build:ts", | ||
| "build": "yarn build:babel && yarn build:ts", |
Author
There was a problem hiding this comment.
&& has slightly different semantics than ; here: with this change, yarn build:ts will only be executed if yarn build:babel succeeds, while it used to be always executed, independently of the return code of yarn build:babel.
However, && is supported cross-platform, while the ; syntax is not.
Considering that stopping the command is probably a sensible thing to do if the babel build fails, I believe that is an acceptable change.
Member
There was a problem hiding this comment.
nice catch! thanks for your diligent work on this PR @timosta and giving us Windows support 👍
Collaborator
|
PatternFly-React preview: https://1251-pr-patternfly-react-patternfly.surge.sh |
Pull Request Test Coverage Report for Build 4233
💛 - Coveralls |
dlabaj
approved these changes
Feb 1, 2019
priley86
approved these changes
Feb 1, 2019
This was referenced Mar 12, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What:
With the current setup, Patternfly's npm scripts cannot be executed in a Windows environment.
The changes proposed by this PR should make all npm scripts platform independent and runnable on both Windows and Linux/OS X environments.
How:
shxto make Unix specific commands platform independent: Commands likerm,cpetc. that are used in PF's npm scripts are not supported by Windows command lines. Theshxcommand line tool provides a layer of abstraction that makes basic unix commands work cross-platform with the established Unix syntax.cross-varto make Unix specific command line variable syntax platform independent: The PF3 packages use Unix style variables$variableNameto refer to environment variables in their npm scripts to build SASS style sheets. Under Windows, a different syntax must be used to make those variables work. Thecross-varcommand line tool provides a layer of abstraction that makes Unix style variables work cross-platform.Open Points/Concerns:
ThePowerShell supports Unix style pipe operationscoverallsscript in the root package.json: It uses a pipe operator, which isn't supported on Windows as-is.|, only cmd does not. Go Powershell! 💪Performance: The additional layer of abstraction for some scripts might cause some additional runtime overhead during the build process. Compare avg. Jenkins build runtimes before and after the change to get an idea of the impact?Comparing the Travis build time including the changes with similar recent PR builds (one, and another one) shows no significant preformance degradations. All are around the 25 to 30 minute mark.Executing Test suite under Windows environment: While the build works fine, running the Jest test suite under Windows currently produces a whole bunch of errors that do not show up on the Travis build. Investigating and resolving this should probably be done in a separate PR.Resolved with fix(build): Support running the test suite under Windows #1255.