tools: add remark dependencies #16635

watilde · 2017-10-31T06:28:54Z

Summary

node_modules is git checked-in for ESLint, but not for Remark.

Fixes #16628.

cc @gibfahn @maclover7

Checklist

make -j4 test
commit message follows commit guidelines

Affected core subsystem(s)

build, tools

joyeecheung

Rubber-stamp LGTM

joyeecheung · 2017-10-31T06:32:04Z

Linter CI: https://ci.nodejs.org/job/node-test-linter/13127/

MylesBorins

LGTM

were there any other files not included in the initial PR?

watilde · 2017-10-31T08:14:28Z

No, only node_modules were excluded in that.

were there any other files not included in the initial PR?

gibfahn · 2017-10-31T10:27:03Z

Makefile

-		cd tools/remark-cli && ../../$(NODE) ../../$(NPM) install; fi
-	if [ ! -d tools/remark-preset-lint-node/node_modules ]; then \
-		cd tools/remark-preset-lint-node && ../../$(NODE) ../../$(NPM) install; fi
+	if [ ! -d tools/remark-cli/node_modules/fsevents/lib ]; then \


This will try to rebuild on non-macOS every time right? I think fsevents is only needed on macOS, so this could be:

if [ "$(OSTYPE)" = darwin -a ! -d tools/remark-cli/node_modules/fsevents/lib ]; then \

That seems to be right. I will replace this line with your comment.

https://github.com/strongloop/fsevents/blob/master/binding.gyp#L6

I've replaced. CI also gets green :)

cjihrig

Rubber stamp LGTM

watilde · 2017-10-31T14:01:56Z

Linter CI: https://ci.nodejs.org/job/node-test-linter/13137/
~~https://ci.nodejs.org/job/node-test-linter/13136/~~

gibfahn · 2017-10-31T14:41:34Z

Makefile

-		cd tools/remark-cli && ../../$(NODE) ../../$(NPM) install; fi
-	if [ ! -d tools/remark-preset-lint-node/node_modules ]; then \
-		cd tools/remark-preset-lint-node && ../../$(NODE) ../../$(NPM) install; fi
+	if [ "$(OSTYPE)" = darwin -a ! -d tools/remark-cli/node_modules/fsevents/lib ]; then \


Actually can you change if to @if, so it doesn't print the output (see #16551).

I expect it's not actually necessary to build. It's only used by Chokidar and that has a fallback.

In that case @watilde maybe just remove this and see whether it still works.

I just removed the line and it's still working. As Ben mentioned, chokidar handles fsevents as an optional dependency. Let's go with it :)

refack · 2017-10-31T15:49:12Z

question: can we webpack this? 2000 additional files is hurting my git performance...

MylesBorins · 2017-11-01T00:37:55Z

Have we done a review of the licenses of the dependencies?

watilde · 2017-11-01T14:24:21Z

Linter CI: https://ci.nodejs.org/job/node-test-linter/13159/

MylesBorins · 2017-11-01T14:44:50Z

I just used license-checker to review licenses

├─ MIT: 181
├─ ISC: 35
├─ BSD-3-Clause: 9
├─ Apache-2.0: 6
├─ MIT*: 3
├─ BSD-2-Clause: 2
├─ (BSD-2-Clause OR MIT OR Apache-2.0): 2
├─ AFLv2.1,BSD: 1
├─ Public Domain: 1
├─ BSD: 1
└─ Unlicense: 1

nothing looks out of the ordinary

watilde · 2017-11-01T16:18:52Z

@refack I think it adds more dependencies if we use remark-loader since it's a wrapper. This diff is a pain indeed, but I can't find the other option.. It was same when we added eslint at #1539 apparently.

refack

Blocking for discussion about optimal dependency managment and CI

Trott · 2017-11-01T22:14:26Z

I could be wrong, but it looks to me like these were installed with:

npm install

but I think we can get a much reduced number of files (and packages) and a working installation with:

npm install --production

Trott · 2017-11-01T22:17:30Z

Have we done a review of the licenses of the dependencies?

I hate to ask, but have we done this for other things where we ship node_modules with our code? (And does it matter in a case like this where it's our tooling and not our actual product? Honest question, I have no idea.) I'm curious but also slightly dreading what happens if we do a license audit on the node_modules in our eslint folder.

refack · 2017-11-01T22:17:52Z

This diff is a pain indeed, but I can't find the other option..

IMHO the current solution is good (keeping just a "recipe" for installing), we need to optimize the CI machine.

refack · 2017-11-01T22:20:09Z

And does it matter in a case like this where it's our tooling and not our actual product? Honest question, I have no idea.

I had same question, discussion in #16640 & #16637 (comment)

watilde · 2017-11-02T00:21:26Z

What I did was: $ npm install --only=prod --ignore-scripts, but I will do a double check.

Trott · 2017-11-02T05:01:44Z

What I did was: $ npm install --only=prod --ignore-scripts, but I will do a double check.

I was basing my comment on the fact that I saw something listed in devDependencies in the mode_modules directory, but now that I think about it, it could have been there as a dependency of a dependency too. Sorry for the noise if I was wrong!

richardlau · 2017-11-02T07:00:04Z

Have we done a review of the licenses of the dependencies?

I hate to ask, but have we done this for other things where we ship node_modules with our code? (And does it matter in a case like this where it's our tooling and not our actual product? Honest question, I have no idea.) I'm curious but also slightly dreading what happens if we do a license audit on the node_modules in our eslint folder.

If we commit something into this repository that came from somewhere else then we have made a copy and are redistributing. At the very least we should be reviewing the license(s) to check that we conform.

watilde · 2017-11-02T13:19:45Z

@Trott oops, no worries that's a good point! I just did double-check of npm install --only=prod --ignore-scripts and it was correct :) No diff appeared.

watilde · 2017-11-06T14:25:52Z

Rebased and fixed conflicts.

Trott · 2017-11-07T11:22:33Z

This would have fixed some issues we had at Code + Learn yesterday. Without this, initial make test requires an internet connection because of the npm install done for remark. I'd love to see that requirement eliminated, so I'm keen to see this land.

watilde · 2017-11-07T13:10:26Z

@refack Do we still need to block? It would be nice if we can continue the discussion regardless of whether the patch is shipped or not :)

gibfahn · 2017-11-07T13:52:14Z

@watilde I think @refacks -1 is because we haven't discussed whether including node_modules is a good idea. If we think it's not a good idea, then it doesn't really make sense to land this PR.

Trott · 2017-11-08T16:24:19Z

@watilde Looks like there's a conflict. Can you rebase?

refack · 2017-11-08T16:30:54Z

@watilde sorry, I'm strongly -1 for adding 2000 new files. It's also sort of "irreversible" since git will always remember these files even if we revert this 🤷‍♂️

Are there other benefits besides faster CI?

MylesBorins · 2017-11-08T16:50:28Z

@refack we have done this with every other dependency, it seems like we need a higher level policy around this if we are going to block specifically on this one

apapirovski · 2017-11-08T16:56:25Z

If this is going to happen, it should happen in the same way it did for eslint. So there should be a script tools/update-remark.sh and we should also run dmn to clean the dependencies. Most of that script could probably be copied with just changing eslint for remark.

Fixes: nodejs#16628

refack · 2017-11-08T17:29:27Z

refack we have done this with every other dependency, it seems like we need a higher level policy around this if we are going to block specifically on this one

@MylesBorins In terms of npm packages precedent it was done for ESLint and tools/doc, it wasn't done for autocannon or wrk which are used for benchmarking. Also this is not a dependency, this is a dev helper tool, as in it's not a requirement for to building a working node binary.
AFAIK there are no other bugs besides the linter CI jobs runs slower. If there are other benefits from this or issues with the status quo I would be happy to reconsider.

refack · 2017-11-08T17:37:19Z

Another data point, the tarball release rule removes these directories:

node/Makefile

Line 833 in d597317

    
           $(RM) -r $(TARNAME)/tools/{eslint,eslint-rules,osx-pkg.pmdoc,pkgsrc,remark-cli,remark-preset-lint-node}

watilde · 2017-11-08T17:56:50Z

Rebased and fixed conflicts.

@refack The other issue is that make test doesn't work in offline without the dependencies of tools. It also means lint tests can be depending on npm registry's status if we don't include them. I think these two are worth enough to care.

It's true that it's really difficult to review, but even if we do not add node_modules, those files will be retrieved vianpm install and will be used, so I thought the things seem to be almost unchanged. Hence, it's better for ci and local development to make running tests faster.

apapirovski

Making it more explicit.

... there should be a script tools/update-remark.sh and we should also run dmn to clean the dependencies. Most of that script could probably be copied with just changing eslint for remark.

If this already exists please let me know. It's almost impossible to check the files changes with 2200 files added.

refack · 2017-11-08T18:08:51Z

@watilde I agree it's a trade off between speed and size/complexity of the workspace.

I just found out that the lint target is conditional on tools/eslint existing, since it's excluded from the tarball

node/Makefile

Lines 1082 to 1107 in d597317

    
           ifneq ("","$(wildcard tools/eslint/)") 
        
           lint: 
        
           	@EXIT_STATUS=0 ; \ 
        
           	$(MAKE) lint-js || EXIT_STATUS=$$? ; \ 
        
           	$(MAKE) lint-cpp || EXIT_STATUS=$$? ; \ 
        
           	$(MAKE) lint-md || EXIT_STATUS=$$? ; \ 
        
           	$(MAKE) lint-addon-docs || EXIT_STATUS=$$? ; \ 
        
           	exit $$EXIT_STATUS 
        
           CONFLICT_RE=^>>>>>>> [0-9A-Fa-f]+|^<<<<<<< [A-Za-z]+ 
        
           lint-ci: lint-js-ci lint-cpp lint-md lint-addon-docs 
        
           	@if ! ( grep -IEqrs "$(CONFLICT_RE)" benchmark deps doc lib src test tools ) \ 
        
           		&& ! ( find . -maxdepth 1 -type f | xargs grep -IEqs "$(CONFLICT_RE)" ); then \ 
        
           		exit 0 ; \ 
        
           	else \ 
        
           		echo "" >&2 ; \ 
        
           		echo "Conflict marker detected in one or more files. Please fix them first." >&2 ; \ 
        
           		exit 1 ; \ 
        
           	fi 
        
           else 
        
           lint: 
        
           	@echo "Linting is not available through the source tarball." 
        
           	@echo "Use the git repo instead:" \ 
        
           		"$ git clone https://github.com/nodejs/node.git" 
        
           lint-ci: lint 
        
           endif

Another option is to use a strategy like shrikpack, that is to check in a zip of the dependencies (maybe using Git LFS), and have a recipe unzip if needed.

MylesBorins · 2017-11-08T19:39:31Z

So it sounds like we are asking for a few different things

update scripts
a better way to manage dependencies

This pull request was opened because the current remark linter that landed broke the ability to test our builds in the tarball

Since we are not reaching consensus about adding the dependencies today would it be possible to open another PR that at the very least fixes the testing while we figure this out?

For the two tasks asked above, an update script and better management of deps... I don't think it is fair to hold this specific dependency to a higher standard than any other we have in the tree. If we want to block on that notion, we can, but we should open another conversation to discuss fixing this problem against all vendored dependencies in the repo

apapirovski · 2017-11-08T19:41:53Z

@MylesBorins I don't think there's anything else in tools/ other than eslint that follows this pattern. I don't think it's unreasonable to expect remark-cli to conform to the same standards. Also, using dmn to clean out the stuff being committed should be a prerequisite even if a script isn't made.

And I think it's reasonable to want this to be done the first time this is added to git because then we don't have to forever carry around files that shouldn't have made it in in the first place.

MylesBorins · 2017-11-08T19:51:41Z

@apapirovski that seems reasonable in regards to the script and running dmn, but not reasonable when it comes to not landing the dependencies.

Either way we should prioritize fixing the test first, I'll see if I can get something together

gibfahn · 2017-11-08T22:23:46Z

This pull request was opened because the current remark linter that landed broke the ability to test our builds in the tarball

I thought it was opened to fix #16628, which isn't a test failure (just a slowdown in the linter CI job). Which test failure is that?

@refack The other issue is that make test doesn't work in offline without the dependencies of tools. It also means lint tests can be depending on npm registry's status if we don't include them. I think these two are worth enough to care.

An alternative option would be a make download, that downloads all the needed "dev" dependencies. So to set up to work offline you'd just do git clone && make download, and you'd get the same results.

refack · 2017-11-08T23:47:29Z

would it be possible to open another PR that at the very least fixes the testing while we figure this out?

Sure, why didn't you ask sooner 😁 #16893

refack · 2017-11-08T23:55:40Z

An alternative option would be a make download, that downloads all the needed "dev" dependencies. So to set up to work offline you'd just do git clone && make download, and you'd get the same results.

So for this scenario the make lint-md-build exists, but it's not an alternative to the build-from-tarball scenario, which is completely offline and integrity verified.
We could ship the tools as a node.tools.tar.gz, but I'm not sure how common this use case is (offline dev environment, as opposed to offline self contained build).

nodejs-github-bot added build Issues and PRs related to build files or the CI. meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory. labels Oct 31, 2017

joyeecheung approved these changes Oct 31, 2017

View reviewed changes

MylesBorins approved these changes Oct 31, 2017

View reviewed changes

gibfahn reviewed Oct 31, 2017

View reviewed changes

cjihrig approved these changes Oct 31, 2017

View reviewed changes

watilde force-pushed the remark-modules branch from f70ebe5 to 364218f Compare October 31, 2017 13:59

gibfahn reviewed Oct 31, 2017

View reviewed changes

gibfahn mentioned this pull request Oct 31, 2017

build: suppress lint-md output #16551

Merged

1 task

watilde force-pushed the remark-modules branch from 364218f to cfc1dc5 Compare November 1, 2017 14:22

refack suggested changes Nov 1, 2017

View reviewed changes

jasnell approved these changes Nov 2, 2017

View reviewed changes

richardlau mentioned this pull request Nov 2, 2017

build: add a OFFLINE var to Makefile #16686

Closed

2 tasks

watilde force-pushed the remark-modules branch from cfc1dc5 to bc5740b Compare November 6, 2017 14:25

Trott approved these changes Nov 7, 2017

View reviewed changes

tools: include remark node_modules

4d77a01

Fixes: nodejs#16628

build: remove lint-md-build

f887f37

watilde force-pushed the remark-modules branch from bc5740b to f887f37 Compare November 8, 2017 17:30

apapirovski requested changes Nov 8, 2017

View reviewed changes

refack mentioned this pull request Nov 8, 2017

tools,build: allow build without remark-cli #16893

Merged

3 tasks

watilde closed this Nov 14, 2017

watilde deleted the remark-modules branch November 14, 2017 21:35

MylesBorins mentioned this pull request Nov 26, 2017

Add remark deps #17315

Closed

gibfahn mentioned this pull request Nov 26, 2017

Add remark dependencies #17320

Closed

refack mentioned this pull request Jul 13, 2018

Produce HTML documentation using unified/remark/rehype #21490

Closed

3 tasks

tools: add remark dependencies #16635

tools: add remark dependencies #16635

Conversation

watilde commented Oct 31, 2017 • edited Loading

Summary

Checklist

Affected core subsystem(s)

joyeecheung left a comment

Choose a reason for hiding this comment

joyeecheung commented Oct 31, 2017

MylesBorins left a comment

Choose a reason for hiding this comment

watilde commented Oct 31, 2017

gibfahn Oct 31, 2017

Choose a reason for hiding this comment

watilde Oct 31, 2017

Choose a reason for hiding this comment

watilde Oct 31, 2017

Choose a reason for hiding this comment

cjihrig left a comment

Choose a reason for hiding this comment

watilde commented Oct 31, 2017 • edited Loading

gibfahn Oct 31, 2017

Choose a reason for hiding this comment

bnoordhuis Oct 31, 2017

Choose a reason for hiding this comment

gibfahn Oct 31, 2017

Choose a reason for hiding this comment

watilde Nov 1, 2017

Choose a reason for hiding this comment

refack commented Oct 31, 2017

MylesBorins commented Nov 1, 2017

watilde commented Nov 1, 2017

MylesBorins commented Nov 1, 2017

watilde commented Nov 1, 2017 • edited Loading

refack left a comment

Choose a reason for hiding this comment

Trott commented Nov 1, 2017 • edited Loading

Trott commented Nov 1, 2017

refack commented Nov 1, 2017

refack commented Nov 1, 2017

watilde commented Nov 2, 2017

Trott commented Nov 2, 2017

richardlau commented Nov 2, 2017

watilde commented Nov 2, 2017

watilde commented Nov 6, 2017

Trott commented Nov 7, 2017

watilde commented Nov 7, 2017

gibfahn commented Nov 7, 2017

Trott commented Nov 8, 2017

refack commented Nov 8, 2017

MylesBorins commented Nov 8, 2017

apapirovski commented Nov 8, 2017

refack commented Nov 8, 2017

refack commented Nov 8, 2017

watilde commented Nov 8, 2017

apapirovski left a comment

Choose a reason for hiding this comment

refack commented Nov 8, 2017

MylesBorins commented Nov 8, 2017

apapirovski commented Nov 8, 2017 • edited Loading

MylesBorins commented Nov 8, 2017

gibfahn commented Nov 8, 2017

refack commented Nov 8, 2017

refack commented Nov 8, 2017

watilde commented Oct 31, 2017 •

edited

Loading

watilde commented Oct 31, 2017 •

edited

Loading

watilde commented Nov 1, 2017 •

edited

Loading

Trott commented Nov 1, 2017 •

edited

Loading

apapirovski commented Nov 8, 2017 •

edited

Loading