tools: add remark dependencies

[watilde]

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]

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]

No, only node_modules were excluded in that.

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

[gibfahn]

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\ 

[watilde]

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

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

[watilde]

I've replaced. CI also gets green :)

[cjihrig]

Rubber stamp LGTM

[watilde]

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

[gibfahn]

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

[bnoordhuis]

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

[gibfahn]

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

[watilde]

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]

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

[MylesBorins]

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

[watilde]

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

[MylesBorins]

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]

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

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]

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]

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]

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]

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

[Trott]

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]

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]

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

Rebased and fixed conflicts.

[Trott]

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]

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

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

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

[refack]

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

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

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.

[refack]

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]

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]

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]

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

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]

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

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

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]

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]

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