build: search missing dependencies on lint-md

[estrada9166]

This will search if there are some dependencies that are missing.
It will run with lint-md
Fixes: #18981
Fixes: #19189 (comment)
Fixes: #18978

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines
  MakeFile

[joyeecheung]

Can you do the same for tools/remark-cli/package.json? Thanks!

[Trott]

@nodejs/build

[gibfahn]

This seems a bit complex, couldn't we just print a warning if the md linter fails that says that you might need to run lint-md-build to update your remark dependencies?

(I haven't looked at this in great detail, so it's quite possible that wouldn't work, let me know if so!)

[estrada9166]

@joyeecheung @Trott @gibfahn Any comments about this? Thanks!!

[Trott]

@joyeecheung @Trott @gibfahn Any comments about this? Thanks!!

I usually defer to others on Makefile stuff (which is why I pinged @nodejs/build). Two other people (besides @joyeecheung already pinged above) who have worked on Makefile a bit in the past few months: @bnoordhuis @refack

Not sure what people will think of pulling package.json dependencies out with a pile of sed commands and/or what alternatives might be suggested. (I imagine the Makefile can't assume npm is installed, for example.)

If no one reviews this, I'll take a closer look and provide feedback or approve, but I think there are more qualified people than me to comment on Makefile changes.

[joyeecheung]

I don't think it's necessary to print every missing dependencies since the user is only going to run make lint-md-build to fix it instead of running npm install commands individually, but I am not against it either.

[gibfahn]

@joyeecheung @Trott @gibfahn Any comments about this? Thanks!!

Do you mean in addition to this one?

This seems a bit complex, couldn't we just print a warning if the md linter fails that says that you might need to run lint-md-build to update your remark dependencies?

[estrada9166]

@gibfahn Sorry the lateness,

This seems a bit complex, couldn't we just print a warning if the md linter fails that says that you might need to run lint-md-build to update your remark dependencies?

If there is an error in the file .remarkrc we are not going to log the error properly and it will say "run lint-md-build" but it would not solve the issue...

[estrada9166]

ping @gibfahn

[refack]

Hello @estrada9166, IMHO as well, this a fairly complex solution. Makefile is already too complex to be easily managed. Also there is already an, admittedly less subtle, solution in the code with make lint-md-build or even make lint-md-clean && make lint-md-build.

If I think about the problem space, a possible simpler solution would be to differentiate tool failure (such as a missing dep) from lint errors, and providing better feedback. If you manage to implement that, I would rather support that.

Bottom line, when evaluating cost/benefit of the current solution, I'm don't feel it's worth the added complexity for fixing an edge case with a side tool 🤷‍♂️

[refack]

IMHO the current solution is too complex for the problem at hand

[gibfahn]

If there is an error in the file .remarkrc we are not going to log the error properly and it will say "run lint-md-build" but it would not solve the issue...

Okay, to check I'm understanding what you're saying,

If one of the dependencies fails to install, (e.g. you do make lint-md-build; rm -r tools/remark-preset-lint-node/node_modules/plur), then when you run make lint-md you'll get lots of errors. Running make lint-md-build won't help because it won't remove the node_modules and reinstall. I can see that that could be a problem.

I also take your point that if we do what I suggested and always say "try running lint-md-clean && lint-md-build" then we'd be giving that suggestion even if there was a legitimate docs error.

I understand that that's an issue, but I basically agree with @refack that this is a lot of complexity in a Makefile that is already very complex.

[gibfahn]

In the discussion following #18981 (comment), decision was that making lint-md-build a dependency of lint-md is a bad idea, because when lint-md-build hasn't been run before lint-md should skip and return nothing rather than trying to npm install (and failing if you're offline).

However, in the case we're talking about here, lint-md-build has already been run before, so the linting won't be skipped.

I don't think Makefiles have a concept of an optional dependency, but basically we could say that lint-md should depend on lint-md-build only in the ifneq ("","$(wildcard tools/remark-cli/node_modules/)") case.

So maybe the answer would be to run lint-md-build if that if condition passes, it takes about 0.4s on my machine, so shouldn't add too much overhead (and it works offline).

[refack]

Another option is to refactor out all of the doc linting (maybe even the doc building) into a seperate file. IMHO that would be a valuable contribution, but that might also be contentious, unless the UX will be kept simple as it is now.

[estrada9166]

My biggest concern is the case that happened, and it is when a new remark-lint dependency is installed; if a new dependency is installed and you pull the code, it would not pull the new package on node_modules so...

I don't think Makefiles have a concept of an optional dependency, but basically we could say that lint-md should depend on lint-md-build only in the ifneq ("","$(wildcard tools/remark-cli/node_modules/)") case.

it will pass and it will throw the well-known error.
The main reason I implemented this approach was that if there's a new remark-lint dependency and you already have the node_modules without the new one, you will not know the reason of the error at the beginning, making you debug it, like in this case

[gibfahn]

My biggest concern is the case that happened, and it is when a new remark-lint dependency is installed; if a new dependency is installed and you pull the code, it would not pull de new package on node_modules so...

I don't think that's true, the remark-lint package.json is checked into git, and @joyeecheung's PR means that if the package.json changes then lint-md-build will rerun npm install in that directory, which should install the new packages (see #18981 (comment)).

[estrada9166]

@gibfahn I'm not sure that the issue is solved at all, check this issue

[gibfahn]

@gibfahn I'm not sure that the issue is solved at all, check this issue

What I'm saying is that make lint-md-build will resolve that issue, so this PR can just have make lint-md always call make lint-md-build, which will no-op normally, but will update the node_modules if there has been a change in the package.json.

[estrada9166]

Just to check if I understand, the suggestion is to call make lint-md-build every time that make lint-md is call?

[gibfahn]

Just to check if I understand, the suggestion is to call make lint-md-build every time that make lint-md is call?

My suggestion (emphasis on the my, others may disagree) is that we do it if the ifneq condition is true.

[estrada9166]

The only thing with ifneq is that if we check node_modules; it still pass if the node_modules folder exists but is missing a dependency.

And running make lint-md-build every time, is avoided for this

[gibfahn]

The only thing with ifneq is that if we check node_modules; it still pass if the node_modules folder exists but is missing a dependency.

And running make lint-md-build every time, is avoided for this

Okay, let me explain myself clearly, and you can tell me if it makes sense.

My proposal is roughly:

ifneq ("","$(wildcard tools/remark-cli/node_modules/)")
+  make lint-md-build # Probably won't look exactly like this
else
# Still skips as it does on master
endif

There are two conditions:

1. Have you run lint-md-build at least once (i.e. does the node_modules folder exist), this is true if the ifneq passes.
2. Has the package.json has changed since you last ran lint-md-build (whether your node_modules is out of date).

If 1. is false, then we should just skip, and not try to run lint-md-build for you. This is handled because the else will fire, so we won't run it.

If 1. is true then we are always okay to run lint-md-build, because

• either: 2. is false, your node_modules are up to date, and the lint-md-build will do nothing.
• or: 2. is true, your node_modules are not up to date, and you need to run lint-md-build, otherwise your lint will fail.

If 1. is true and 2. is true, then running lint-md-build should fix your node_modules, as @joyeecheung explained in #18981 (comment).

[estrada9166]

I can make the change to run make lint-md-build every time that make lint-md is call; the only thing is this comment:

^ That's true. We tried to avoid internet access.
Context: #16628

[BridgeAR]

This will likely be superseded by #20109

[jasnell]

What's the current status on this one?

[Trott]

I think this can be closed as the issues that it addresses have been closed and an alternate solution appears to have landed. Thanks for the pull request, @estrada9166, and if I'm wrong about this and this should remain open, leave a comment (or re-open it if GitHub presents a re-open button).