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

tools: try installing js-yaml only once #16661

Closed
wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Nov 1, 2017

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!

@@ -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

@joyeecheung
Copy link
Member Author

joyeecheung commented Nov 17, 2017

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
Copy link
Member Author

Landed in f24d961, thanks!

joyeecheung added a commit that referenced this pull request Nov 17, 2017
PR-URL: #16661
Fixes: #16650
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
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
gibfahn pushed a commit that referenced this pull request Dec 19, 2017
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
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI.
Projects
None yet
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