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.

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(build): Support running npm scripts under Windows #1251

Merged
merged 3 commits into from
Feb 1, 2019
Merged

fix(build): Support running npm scripts under Windows #1251

merged 3 commits into from
Feb 1, 2019

Conversation

TimoStaudinger
Copy link

@TimoStaudinger TimoStaudinger commented Jan 25, 2019

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:

  • Use shx to make Unix specific commands platform independent: Commands like rm, cp etc. that are used in PF's npm scripts are not supported by Windows command lines. The shx command line tool provides a layer of abstraction that makes basic unix commands work cross-platform with the established Unix syntax.
  • Use cross-var to make Unix specific command line variable syntax platform independent: The PF3 packages use Unix style variables $variableName to 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. The cross-var command line tool provides a layer of abstraction that makes Unix style variables work cross-platform.

Open Points/Concerns:

  • The coveralls script in the root package.json: It uses a pipe operator, which isn't supported on Windows as-is. PowerShell supports Unix style pipe operations |, 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.
  • Test changes under Unix environment: I currently only have access to a Windows environment. While it should work just fine under a Unix one, this needs to be verifed.

@@ -25,7 +25,7 @@
"homepage": "https://github.com/patternfly/patternfly-react#readme",
"scripts": {
"prebuild": "node ./build/generateIcons.js",
"build": "yarn build:babel; yarn build:ts",
"build": "yarn build:babel && yarn build:ts",
Copy link
Author

Choose a reason for hiding this comment

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

&& 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.

Copy link
Member

Choose a reason for hiding this comment

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

nice catch! thanks for your diligent work on this PR @timosta and giving us Windows support 👍

@TimoStaudinger TimoStaudinger changed the title fix(build): Support running npm scripts under Windows WIP: fix(build): Support running npm scripts under Windows Jan 25, 2019
@patternfly-build
Copy link
Contributor

PatternFly-React preview: https://1251-pr-patternfly-react-patternfly.surge.sh

@coveralls
Copy link

coveralls commented Jan 25, 2019

Pull Request Test Coverage Report for Build 4233

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 80.196%

Totals Coverage Status
Change from base Build 4223: 0.0%
Covered Lines: 4624
Relevant Lines: 5416

💛 - Coveralls

@TimoStaudinger TimoStaudinger changed the title WIP: fix(build): Support running npm scripts under Windows fix(build): Support running npm scripts under Windows Jan 25, 2019
Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

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

LGTM

@dlabaj dlabaj merged commit df0c275 into patternfly:master Feb 1, 2019
@TimoStaudinger TimoStaudinger deleted the fix/windows-npm-scripts branch February 1, 2019 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants