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/doc: add files to gitignore #17224

Merged
merged 1 commit into from
Nov 24, 2017

Conversation

RichardLitt
Copy link
Contributor

This closes #17216 by adding some files to the gitignore which stick around after an aborted build. It is meant as a temporary fix.

Checklist
Affected core subsystem(s)

.gitignore, but related to docs.

@nodejs-github-bot nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label Nov 22, 2017
Copy link
Contributor

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

This should be 'tools/doc'

Sorry for confusion

@RichardLitt RichardLitt changed the title doc: add files to gitignore tools/doc: add files to gitignore Nov 22, 2017
@RichardLitt RichardLitt force-pushed the feat/add-files-to-gitignore branch 2 times, most recently from 8791f57 to 82bb669 Compare November 22, 2017 08:53
@RichardLitt
Copy link
Contributor Author

No problem at all. Changed the commit, commit message, and PR title. That work?

@MylesBorins
Copy link
Contributor

@RichardLitt it seems like I may have not properly identified the issue

it seems that there is already a tools/doc/node_modules (I had thought I checked before opening the issue but was mistaken)

The issue appears to be extra files getting added during make test and those showing up in the git diff

attached is an image of this, best I could do as this happened on other systems

img_20171122_163607 1

I'm not sure that this approach with the git ignore is the correct solution, sorry about that. That being said we should use this PR to find the correct solution 😄

I'll update when I can dig in a bit more. Perhaps you can get this to repro on your machine

@joyeecheung
Copy link
Member

joyeecheung commented Nov 22, 2017

About the node_modules: I think we should ignore the tools/doc/node_modules folder, but add back (!) the subfolders/files that we checked into the source code.

As for the package-lock.json...I think we should actually commit that into the source?

Also cc @refack

.gitignore Outdated
@@ -103,6 +103,8 @@ deps/npm/node_modules/.bin/
/SHASUMS*.txt*

# test artifacts
tools/doc/node_modules
Copy link
Member

@joyeecheung joyeecheung Nov 22, 2017

Choose a reason for hiding this comment

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

So I think this should be

tools/doc/node_modules/*
!tools/doc/node_modules/.bin/marked
!tools/doc/node_modules/js-yaml/index.js
!tools/doc/node_modules/marked

And commit the package-lock.json into the source instead of ignoring it.

@RichardLitt
Copy link
Contributor Author

I edited the commit to include @joyeecheung's suggestions. I also committed the package-lock.json file.

Should be good?

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

LGTM but I would like @watilde @refack to take a look

@refack
Copy link
Contributor

refack commented Nov 22, 2017

I'll take a look, but IMHO if everything is published to npm, removing tools/doc/node_modules and adding package-lock.json is the optimal situation. That might require changing the tests to detect an offline scenario.

@refack
Copy link
Contributor

refack commented Nov 22, 2017

@RichardLitt I pushed a suggested change to this PR. Feel free to remove it.

@RichardLitt
Copy link
Contributor Author

Can't think of a reason to remove it, @refack. No need for every commit in this PR to come from me.

Copy link
Contributor

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

LGTM

@joyeecheung joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 23, 2017
@joyeecheung
Copy link
Member

@Bamieh
Copy link
Contributor

Bamieh commented Nov 23, 2017

There is also a change in a tracked file:

	modified:   tools/doc/node_modules/js-yaml/index.js

image

Ignore the test/fixtures/failcounter.js it is added by me 😄

@refack
Copy link
Contributor

refack commented Nov 23, 2017

@Bamieh did you test with this patch? It should take care of this (actually unneeded) file modification.

@Bamieh
Copy link
Contributor

Bamieh commented Nov 23, 2017

@refack yes i did run the tests and everything is in order

On branch feat/add-files-to-gitignore
Your branch is up-to-date with 'RichardLitt/feat/add-files-to-gitignore'.
nothing to commit, working tree clean

@addaleax
Copy link
Member

Tbh, I don't think we should be running npm install in the first place... this change just works around that issue it seems?

@MylesBorins
Copy link
Contributor

@addaleax while I agree, I don't think we yet have consensus on removing npm install, and in the mean time this is creating friction with the test suite

@refack
Copy link
Contributor

refack commented Nov 24, 2017

I'm thinking of landing this tomorrow morning, then start working on a new PR for further improvements.

@refack refack self-assigned this Nov 24, 2017
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

PR-URL: nodejs#17224
Fixes: nodejs#17216
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@refack refack removed their assignment Nov 24, 2017
@refack refack removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 24, 2017
@refack refack merged commit 887e232 into nodejs:master Nov 24, 2017
@devsnek devsnek mentioned this pull request Nov 24, 2017
3 tasks
@RichardLitt
Copy link
Contributor Author

RichardLitt commented Nov 24, 2017 via email

@MylesBorins MylesBorins mentioned this pull request Nov 26, 2017
"js-yaml": "^3.5.2"
},
"devDependencies": {},
Copy link
Member

Choose a reason for hiding this comment

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

Why this change? js-yaml is a full dependency of the doctool

Copy link
Contributor

Choose a reason for hiding this comment

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

At some point it you hooked js-yaml to redirect to the copy that is in ESLint.

'use strict';
// Hack to load the js-yaml module from eslint.
// No other reason than that it’s huge.
const path = require('path');
const realJSYaml = path.resolve(
__dirname, '..', '..', '..', // tools/
'eslint',
'node_modules',
'js-yaml'
);
module.exports = require(realJSYaml);

This was overwritten anytime npm i was run.
(I agree we should also stop running npm i as part of the regular flows, since tools/doc should work as is in git)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, not running npm i is the fix here. It’s too bad the plans for releasing the doctool as a standalone module didn’t get anywhere so far, but semantically this is backwards :(

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. I'll roll this back as soon as I finish testing the install-les make target.

MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
PR-URL: #17224
Fixes: #17216
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
PR-URL: #17224
Fixes: #17216
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
gibfahn pushed a commit that referenced this pull request Jan 3, 2018
PR-URL: #17224
Fixes: #17216
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@gibfahn
Copy link
Member

gibfahn commented Jan 3, 2018

Had to pull this into v8.9.4 as the lack of it was breaking the macOS doc-upload build job.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

if test suite is aborted early tools/doc/node_modules and tools/doc/package-lock.json are not removed