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

tools: try installing js-yaml only once #16661

Closed
wants to merge 1 commit into from
Closed

tools: try installing js-yaml only once #16661

wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Nov 1, 2017 •
edited
Loading

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

tools

Fixes: #16650

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Nov 1, 2017
@joyeecheung
Copy link
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/11126/

do we have a CI for running make test on a source tarball?

@joyeecheung
Copy link
Member Author

Failures look unrelated

@gibfahn
Copy link
Member

gibfahn commented Nov 1, 2017

do we have a CI for running make test on a source tarball?

No, but we definitely should!

@joyeecheung
Copy link
Member Author

Ping @nodejs/collaborators can someone review this? Thanks!

richardlau
richardlau reviewed Nov 7, 2017
View reviewed changes
Makefile
@@ -561,15 +562,16 @@ gen-json = tools/doc/generate.js --format=json $< > $@
gen-html = tools/doc/generate.js --node-version=$(FULLVERSION) --format=html \
--template=doc/template.html --analytics=$(DOCS_ANALYTICS) $< > $@

gen-doc = \
install-yaml:
Copy link
Member

Choose a reason for hiding this comment

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

Please add install-yaml to the list of phony targets.

Copy link
Contributor

Choose a reason for hiding this comment

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

does doc-targets need to be added as well?

@Trott
Copy link
Member

Trott commented Nov 8, 2017

@nodejs/build

@rvagg
Copy link
Member

rvagg commented Nov 11, 2017

pretty weird, I don't really understand why this makes a difference but if it works @joyeecheung then you can have my lgtm

@joyeecheung
Copy link
Member Author

Added js-yaml to the phony targets per @richardlau 's suggestion. PTAL.

CI: https://ci.nodejs.org/job/node-test-pull-request/11373/

@rvagg

pretty weird, I don't really understand why this makes a difference but if it works @joyeecheung then you can have my lgtm

I am not quite sure either but I can be certain this is related to GNU make, because I can reproduce that bug with gmake but not with make on mac os.

@joyeecheung
Copy link
Member Author

I think I have a rough idea about why this fixes the issue now, it probably has something to do with the gen-doc definition, the bash conditional there is probably interpreted differently in GNU make so moving the conditionals to a separate place somehow works around the compatibility issue. I'll need to dig into that deeper to fix install-yaml properly later, but at least the current patch seem to work.

The pis are not working properly at the moment, this just need a green arm CI before landing.

@joyeecheung
Copy link
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/11477/

@joyeecheung
Copy link
Member Author

joyeecheung commented Nov 17, 2017 •
edited
Loading

Smart OS failed, possibly related to #17043 (this time the test/addons/ are missing builds) , still try again to be safe..

New SmartOS CI: https://ci.nodejs.org/job/node-test-commit-smartos/13195/

@joyeecheung joyeecheung mentioned this pull request Nov 17, 2017
Closed
4 tasks
@joyeecheung
Copy link
Member Author

Landed in f24d961, thanks!

joyeecheung added a commit that referenced this pull request Nov 17, 2017
@joyeecheung
tools: try installing js-yaml only once
f24d961 
PR-URL: #16661
Fixes: #16650
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
This was referenced Nov 17, 2017
Closed
Closed
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@joyeecheung @MylesBorins
tools: try installing js-yaml only once
7fe6a8f 
PR-URL: #16661
Fixes: #16650
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
Merged
gibfahn pushed a commit that referenced this pull request Dec 19, 2017
@joyeecheung @gibfahn
tools: try installing js-yaml only once
962fd75 
PR-URL: #16661
Fixes: #16650
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
Closed
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
@joyeecheung @gibfahn
tools: try installing js-yaml only once
8319b68 
PR-URL: #16661
Fixes: #16650
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@refack refack refack left review comments

@richardlau richardlau richardlau approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

test: Node 8.9.0 cannot 'make test' when building from source code
9 participants
@joyeecheung @gibfahn @Trott @rvagg @refack @jasnell @richardlau @MylesBorins @nodejs-github-bot