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

FLUID-6338: Moving buildStylus from postInstall to prepare. #931

Merged
merged 3 commits into from Sep 13, 2018

Conversation

Projects
None yet
4 participants
@jobara
Copy link
Member

commented Sep 10, 2018

This should prevent it from trying to run buildStylus when installed as
a dependency of another package.

Should also using Infusion as a dependency in another project using 3.0.0-dev.20180913T134630Z.1dc9abaf4.FLUID-6338

https://issues.fluidproject.org/browse/FLUID-6338

FLUID-6338: Moving buildStylus from postInstall to prepare.
This should prevent it from trying to run buildStylus when installed as 
a dependency of another package.
@incd-ci-robot

This comment has been minimized.

Copy link

commented Sep 10, 2018

Could one of the admins verify that these changes are reasonable to test? If so, please reply with "ok to test".

@jobara

This comment has been minimized.

Copy link
Member Author

commented Sep 10, 2018

ok to test

1 similar comment
@waharnum

This comment has been minimized.

Copy link
Member

commented Sep 11, 2018

ok to test

@incd-ci-robot

This comment has been minimized.

@waharnum

This comment has been minimized.

Copy link
Member

commented Sep 11, 2018

ok to test

@incd-ci-robot

This comment has been minimized.

@waharnum

This comment has been minimized.

Copy link
Member

commented Sep 11, 2018

@jobara The separated panel tests appear to be failing with this change (https://ci.fluidproject.org/job/infusion-pr-browser-tests/37/console) - is prepare expected to run following an NPM install automatically?

@jobara

This comment has been minimized.

Copy link
Member Author

commented Sep 11, 2018

@waharnum yes, it should be the same as the old prepublish script "prepare: Run both BEFORE the package is packed and published, and on local npm install without any arguments (See below). This is run AFTER prepublish, but BEFORE prepublishOnly." ( https://docs.npmjs.com/misc/scripts )

@jobara

This comment has been minimized.

Copy link
Member Author

commented Sep 11, 2018

ok to test

@incd-ci-robot

This comment has been minimized.

@jobara

This comment has been minimized.

Copy link
Member Author

commented Sep 11, 2018

@waharnum I did a fresh clone on my own machine and ran npm install. The stylus files were built. I ran the tests as well using npm test and they passed. @avtar suggested it may be related to https://issues.jenkins-ci.org/browse/JENKINS-35170 but we're not sure.

@jobara

This comment has been minimized.

Copy link
Member Author

commented Sep 12, 2018

ok to test

@incd-ci-robot

This comment has been minimized.

@jobara

This comment has been minimized.

Copy link
Member Author

commented Sep 12, 2018

ok to test

@incd-ci-robot

This comment has been minimized.

@incd-ci-robot

This comment has been minimized.

@jobara

This comment has been minimized.

Copy link
Member Author

commented Sep 12, 2018

@waharnum, @avtar There is an npm issue where the prepare step doesn't run when installing from a branch. I wonder if the issue with the CI is related to that. I'm not exactly sure how the PR is fetched though, so can't confirm. npm/npm#19684

@waharnum

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

@jobara @avtar I'll have to look into this more closely at the VM level - I believe the PR Builder plugin uses commit hashes rather than branches, which does work according to the NPM issue as I'm reading it. Will try to investigate today.

@waharnum

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

ok to test

@waharnum

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

I've jumped into the VM on the current build (https://ci.fluidproject.org/job/infusion-pull-request/47/) and manually run grunt buildStylus - will see if this makes a difference.

@incd-ci-robot

This comment has been minimized.

@waharnum

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

Doesn't seem to have made a difference, investigating the Vagrant setup locally now.

@waharnum

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

Tests on this branch are passing from Vagrant locally, both within the VM directly and from outside it via npm run test:vagrantBrowser - doing a wipe on the Jenkins workspace to make sure it's starting from fresh and trying again.

@waharnum

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

ok to test

@incd-ci-robot

This comment has been minimized.

@jobara

This comment has been minimized.

Copy link
Member Author

commented Sep 13, 2018

@waharnum I think I've found the problem. I used the vagrant setup that comes with the repo and found that it also wasn't working. Took a few tries because I had to remember to clear out the existing built css before running vagrant up. What I've found is the VM is running NPM v3.10.10. However, the prepare script wasn't added till NPM v4. How can we update NPM in the VM?

FLUID-6338: Unpinning npm version.
Need at least NPM v4 to run the prepare step.

@jobara jobara force-pushed the jobara:FLUID-6338 branch from 43300c7 to 1dc9aba Sep 13, 2018

@incd-ci-robot

This comment has been minimized.

@cindyli cindyli merged commit 1dc9aba into fluid-project:master Sep 13, 2018

2 checks passed

jenkins Build finished.
Details
license/cla Contributor License Agreement is signed.
Details
@cindyli

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

Merged at 4a66b15

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.