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: update Markdown linter to be cross-platform #31239

Closed
wants to merge 2 commits into from
Closed

tools: update Markdown linter to be cross-platform #31239

wants to merge 2 commits into from

Conversation

DerekNonGeneric
Copy link
Contributor

@DerekNonGeneric DerekNonGeneric commented Jan 7, 2020

Prior to this commit, the dependencies were not
matching the build procedure. This has been
corrected and it has the added benefit of being
able to be built on Windows as well.

  • continue using rollup rather than ncc
  • do not require fs-events for non-macOS
  • use npx and shx for cross-platform building
  • ensure lint-md-rollup runs before lint-md

/cc @Trott

Checklist

@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Jan 7, 2020
@DerekNonGeneric
Copy link
Contributor Author

A little more information might be useful. This PR uses the original way of bundling with only slight modifications to its code. ncc is replaced by rollup so that it is able to be built on Windows. I've removed the shebang due to Rollup complaining. I'm not sure if this was being executed without Node like ./lint-md.js instead of node lint-md.js, but if that's the case theoretically, I could re-add it with rollup-plugin-add-shebang. Please let me know if that's desired.

@Trott
Copy link
Member

Trott commented Jan 9, 2020

Relevant failure on Travis:

/home/travis/build/nodejs/node/tools/node-lint-md-cli-rollup/rollup.config.js
  1:1  error  Parsing error: 'import' and 'export' may appear only with 'sourceType: "module"'
> 1 | import resolve from '@rollup/plugin-node-resolve';
    | ^
  2 | import commonjs from '@rollup/plugin-commonjs';
  3 | import json from '@rollup/plugin-json';
  4 | 
✖ 1 problem (1 error, 0 warnings)

@Trott
Copy link
Member

Trott commented Jan 9, 2020

ESLint is still failing (run make lint-js locally--vcbuild lint-js on Windows).

/home/travis/build/nodejs/node/tools/node-lint-md-cli-rollup/rollup.config.js
  39:13  error  Strings must use singlequote  quotes
✖ 1 problem (1 error, 0 warnings)
  1 error and 0 warnings potentially fixable with the `--fix` option.

@Trott
Copy link
Member

Trott commented Jan 9, 2020

The Travis markdown linting is failing because lint-md.js now needs to be built. It runs make lint-py doc-only lint which tries to use lint-md.js without building it. That works before this change because lint-md.js is checked in but breaks with this change.

@DerekNonGeneric DerekNonGeneric changed the title tools: update Markdown linter to use Rollup tools: update Markdown linter to be cross-platform Jan 13, 2020
@DerekNonGeneric
Copy link
Contributor Author

DerekNonGeneric commented Jan 13, 2020

/to @Trott

The Travis markdown linting is failing because lint-md.js now needs to be built.

Sorry about that! I forgot that build products were being committed.

I've tested this to be working on:

  • Debian Linux with Bash
  • Windows Server with Powershell

Verify on Linux

  1. Run make lint-md-rollup from the repo root dir to build it
  2. Run make lint-md from the repo root dir to run the task

Note: these steps should also work on macOS, although I haven't tested.

Verify on Windows

Note: there is no make command on Windows.

  1. Change dir into tools/node-md-rollup
  2. Run npm run build-node to build it
  3. Change dir back to the repo root
  4. Run node tools/lint-md.js --help to ensure it works

@Trott
Copy link
Member

Trott commented Jan 14, 2020

Perhaps all that's necessary to have this passing again on Travis would be to add the lint-md-rollup task somewhere in these lines of the Travis config file?

@DerekNonGeneric
Copy link
Contributor Author

DerekNonGeneric commented Jan 14, 2020

/to @Trott

Perhaps all that's necessary to have this passing again on Travis [...]

It seems to have passed all the CI checks once I re-committed the bundled build product. It now says "All checks have passed 5 successful checks." Is there another check that I should be looking at?

P.S. I could not get .\vcbuild lint-js to run because I think I have the wrong version of Python installed, but it shouldn't be necessary to ensure that the linter is functional using the procedure described above.

@DerekNonGeneric
Copy link
Contributor Author

@Trott, after looking at this again, it seems like you may have been interested in avoiding the build product being committed. I have never been a fan of committing build products as the alternative is much cleaner. The CI (and others who use the linter) would then be expected to build the bundle themselves. There are actually three different options (and a bonus) that I can see at this point:

  1. Merge as is (commit the 44,772 line bundle) 👎
  2. Minify the bundle then merge (commit a 1 line bundle) 👎
  3. Add the linter's build step to CI + instructions to docs (don't commit the bundle) 👍

My preference is option 3 with the only drawback being that it may take a bit longer to get the linter to be operational.

Bonus (CI only): use Travis Workspaces (Beta) to use a previous build's artifact of the bundle avoiding the need to rebuild it each time. This can even be the minified bundle if that saves transfer time.

Please let me know what you decide!

@Trott
Copy link
Member

Trott commented Jan 18, 2020

it seems like you may have been interested in avoiding the build product being committed.

My suggestion was to add the build step to Travis rather than commit the build product, although committing the build product would be continuing what we currently do and I wouldn't oppose it.

3. Add the linter's build step to CI + instructions to docs (don't commit the bundle) 👍

This is what I meant when I wrote:

Perhaps all that's necessary to have this passing again on Travis would be to add the lint-md-rollup task somewhere in these lines of the Travis config file?

In other words, add the necessary build step to the Travis config file.

My preference is option 3

Same!

the only drawback being that it may take a bit longer

The build step takes seconds on my laptop and I can't imagine it would be much slower in CI. I'm still all for option 3!

@Trott
Copy link
Member

Trott commented Jan 18, 2020

(Also, this needs a rebase because we updated remark-preset-lint-node recently. Sorry/thanks!)

@DerekNonGeneric
Copy link
Contributor Author

DerekNonGeneric commented Jan 20, 2020

@Trott, all checks have passed. I did not include any additional documentation recommending to run make lint-md-rollup prior to running make lint since I think the recommendation is to run make test-doc, which includes lint-md-rollup.

@DerekNonGeneric
Copy link
Contributor Author

@Trott, I've tied up all loose ends on this PR. What follows are the results of all remaining inquiries.

P.S. I could not get .\vcbuild lint-js to run [...]

I was able to confirm that these all work flawlessly:

  • .\vcbuild lint
  • .\vcbuild lint-js
  • .\vcbuild lint-md

The command to build the linter on Windows doesn't currently exist:

  • .\vcbuild lint-md-rollup

(I'm not sure this is necessary.)

I did not include any additional documentation [...]

Currently, no documentation is instructing people to build the Markdown linter from the source. However, this is OK because the committed bundle works out of the box all majors OSes.

I've tested this to be true for:

  • Windows 10
  • macOS
  • Debian 10

As you can see, all CI checks have passed which means that it concurs.

With my most recent fixup, I've ensured hat the linter is able to be built from source on all three of these OSes too.

Also, this needs a rebase [...]

I took care of this as well.

It might be nice to drive this one home soon enough to be included in #31368 (along with #30216) to ensure that the Markdown linter is fully buildable and operational on all platforms.

Prior to this commit, the dependencies were not
matching the build procedure. This has been
corrected and it has the added benefit of being
able to be built on Windows as well.

* continue using `rollup` rather than `ncc`
* do not require `fs-event`s for non-macOS
* use `npx` and `shx` for cross-platform building
* ensure `lint-md-rollup` runs before `lint-md`
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Can we remove tools/lint-md.js before landing since the Maekfile will build it in the course of a normal test-doc build?

@Trott
Copy link
Member

Trott commented Feb 2, 2020

Can we remove tools/lint-md.js before landing since the Maekfile will build it in the course of a normal test-doc build?

Or else can we get rid of make lint-md-rollup in the .travis.yml file? It seems like we can either include the rolled up lint-md.js in the repo or else have everything build it, but it looks like we're doing both.

@DerekNonGeneric
Copy link
Contributor Author

DerekNonGeneric commented Feb 2, 2020

It looks like we're doing both.

You're right about that. I think the best option would be to stop running the command to build it during installation since the committed bundle works cross-platform anyway. Does that sound good to you? I think the only functionality that would require this to be re-built on another platform would be if someone is doing file watching (fsevents macOS problem). I don't think this ever takes place during the entire linting process.

@Trott
Copy link
Member

Trott commented Feb 2, 2020

. I think the best option would be to stop running the command to build it during installation since the committed bundle works cross-platform anyway. Does that sound good to you?

Yes. Then it will be a lot easier to figure out if vcbuild lint-md works as expected on Windows. :-D

@DerekNonGeneric
Copy link
Contributor Author

/to @Trott

Can we remove tools/lint-md.js before landing since the Maekfile will build it in the course of a normal test-doc build?

If this were to happen, we'd need to include a build task for vcbuild.dat on Windows (also determine at which point it needs to run). Let me know, your call!

@Trott
Copy link
Member

Trott commented Feb 5, 2020

/to @Trott

Can we remove tools/lint-md.js before landing since the Maekfile will build it in the course of a normal test-doc build?

If this were to happen, we'd need to include a build task for vcbuild.dat on Windows (also determine at which point it needs to run). Let me know, your call!

Yeah, I think the thing to do is stop running the md-lint-build step as part of linting and just keep lint-md.js in the repository.

@DerekNonGeneric
Copy link
Contributor Author

DerekNonGeneric commented Feb 5, 2020

@Trott, the PR currently reflects the behavior you describe — the linter is only built on-demand and is not part of any other tasks.

I'm not sure why my fixup! commit isn't showing in the changed files, but I did take care of it there.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Feb 13, 2020

Landed in a751389

@Trott Trott closed this Feb 13, 2020
Trott pushed a commit that referenced this pull request Feb 13, 2020
Prior to this commit, the dependencies were not
matching the build procedure. This has been
corrected and it has the added benefit of being
able to be built on Windows as well.

* continue using `rollup` rather than `ncc`
* do not require `fs-event`s for non-macOS
* use `npx` and `shx` for cross-platform building
* ensure `lint-md-rollup` runs before `lint-md`

PR-URL: #31239
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this pull request Feb 17, 2020
Prior to this commit, the dependencies were not
matching the build procedure. This has been
corrected and it has the added benefit of being
able to be built on Windows as well.

* continue using `rollup` rather than `ncc`
* do not require `fs-event`s for non-macOS
* use `npx` and `shx` for cross-platform building
* ensure `lint-md-rollup` runs before `lint-md`

PR-URL: #31239
Reviewed-By: Rich Trott <rtrott@gmail.com>
@codebytere codebytere mentioned this pull request Feb 17, 2020
codebytere pushed a commit that referenced this pull request Mar 15, 2020
Prior to this commit, the dependencies were not
matching the build procedure. This has been
corrected and it has the added benefit of being
able to be built on Windows as well.

* continue using `rollup` rather than `ncc`
* do not require `fs-event`s for non-macOS
* use `npx` and `shx` for cross-platform building
* ensure `lint-md-rollup` runs before `lint-md`

PR-URL: #31239
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this pull request Mar 17, 2020
Prior to this commit, the dependencies were not
matching the build procedure. This has been
corrected and it has the added benefit of being
able to be built on Windows as well.

* continue using `rollup` rather than `ncc`
* do not require `fs-event`s for non-macOS
* use `npx` and `shx` for cross-platform building
* ensure `lint-md-rollup` runs before `lint-md`

PR-URL: #31239
Reviewed-By: Rich Trott <rtrott@gmail.com>
@codebytere codebytere mentioned this pull request Mar 17, 2020
codebytere pushed a commit that referenced this pull request Mar 30, 2020
Prior to this commit, the dependencies were not
matching the build procedure. This has been
corrected and it has the added benefit of being
able to be built on Windows as well.

* continue using `rollup` rather than `ncc`
* do not require `fs-event`s for non-macOS
* use `npx` and `shx` for cross-platform building
* ensure `lint-md-rollup` runs before `lint-md`

PR-URL: #31239
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants