tools,build: .gitignore tweaks #17380

refack · 2017-11-28T23:54:58Z

Using git ls-files -i --exclude-standard to find tracked files that match ignore patterns.

Before

https://gist.github.com/refack/62edafbbc788783ca9a7b8ee8354ae1c

After

deps/v8/test/mjsunit/tools/profviz-test.log
deps/v8/test/mjsunit/tools/tickprocessor-test-func-info.log
deps/v8/test/mjsunit/tools/tickprocessor-test.log
tools/doc/package-lock.json
tools/remark-cli/package-lock.json
tools/remark-preset-lint-node/package-lock.json

The mismatch with package-lock.json files is fixed in #17330

Checklist

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

Affected core subsystem(s)

tools,build

MylesBorins

Handful of questions that I think repeat throughout the document

I'll dig through again after getting answers to the first bit

MylesBorins · 2017-11-29T05:09:14Z

.gitignore

-!test/fixtures/**/.*
-!tools/eslint/**/.*
-!tools/doc/node_modules/**/.*
+!deps/**


I'm guessing this would trump #17363

I'll check. exclusion rules seem to be tricky (CSS style), but I think these are stronger:

node/.gitignore

Lines 111 to 120 in a84f58d

# npm stuff

node_modules

package-lock.json

!/deps/npm/node_modules**

/deps/npm/node_modules/node-gyp/gyp/**/*.pyc

!/tools/eslint/node_modules**

!/tools/doc/node_modules**

!/test/fixtures/**

# not needed and causes issues for distro packagers

/deps/npm/node_modules/.bin/

should it at the very least be at the bottom to avoid other rules combatting it?

What I saw while testing is that most of the rules below apply for /deps (only conflict is debug/release), so IMO it makes sense to keep it up here.

gibfahn · 2017-11-29T12:21:31Z

.gitignore

@@ -24,9 +22,8 @@ perf.data.old
 tags
 .lock-wscript
 *.pyc
-doc/api.xml
+/doc/api.xml


Do you mean why was this not an absolute path? This PR makes it one right?

I made all relative paths absolute, since we have all sorts of path patterns deeper in the tree.

MylesBorins · 2017-11-29T05:10:22Z

.gitignore

+!/deps/v8/src/base/debug
+!/deps/v8/src/debug
+/deps/v8/src/debug/obj
+deps/v8/src/inspector/Debug/


if we are whitelisting all of deps/ above do we need to do this here?
Also should we be ignoring anything in the deps tree at all? this seems odd to me

because the debug/ pattern re-ignores all paths that have it in anywhere in the path. So we need to do a checkerboard.

ignore `/debug/

exclude the 3 listed dirs explicitly

but ignore build artifacts within them

Probably should try to upstream renaming those directories from debug to something like debugSrc

RE: the last one I'll double check.

MylesBorins · 2017-11-29T05:11:45Z

Should we simply move !deps/ to the bottom? Should we be ignoring anything in the deps folder?

gibfahn · 2017-11-29T12:21:31Z

.gitignore

@@ -24,9 +22,8 @@ perf.data.old
 tags
 .lock-wscript
 *.pyc
-doc/api.xml
+/doc/api.xml


Do you mean why was this not an absolute path? This PR makes it one right?

refack · 2017-11-29T14:21:30Z

General comments:

I made all relative paths absolute, since we have all sorts of path patterns deeper in the tree.
Can't wildcard exclude /deps/** since building put artifacts within the /deps/ tree, so we need to do some checkerboarding

MylesBorins · 2017-11-29T16:28:41Z

@refack could we explicitly whitelist deps here and put a .gitignore in the deps folder to ignore the specific build artifacts? That seems a bit cleaner and less error prone

MylesBorins

I'm not sure I agree with how we are currently white listing the deps folder. I'd like to see a response to my earlier comment about whitelisting all of deps and introducing a new .gitignore in the deps/ directory

refack · 2017-11-30T00:52:36Z

could we explicitly whitelist deps here and put a .gitignore in the deps folder to ignore the specific build artifacts? That seems a bit cleaner and less error prone

It's the classical single-point-of-change vs. modularity issue...
My main concern is that git still lacks good exclusion traceability, i.e. it's not easy to correlate an actual exclusion with a .gitignore rule. So tracking them down in multiple files will increase that challenge.

AFAICT most of the noise in this file is due to debug/release directories created during build, I'll try to figure out a better strategy to handle those. That might simplify this file extensively.

MylesBorins · 2017-11-30T01:20:25Z

I think a special case for the deps folder including a comment in the top level gitignore stating to look in the folder for the specific rules is a reasonable approach I look forward to seeing what you come up with

…

On Nov 30, 2017 9:52 AM, "Refael Ackermann" ***@***.***> wrote: could we explicitly whitelist deps here and put a .gitignore in the deps folder to ignore the specific build artifacts? That seems a bit cleaner and less error prone It's the classical single-point-of-change vs. modularity issue... My main concern is that git still lacks good exclusion traceability, i.e. it's not easy to correlate an actual exclusion with a .gitignore rule. So tracking them down in multiple files will increase that challenge. AFAICT most of the noise in this file is due to debug/release directories created during build, I'll try to figure out a better strategy to handle those. That might simplify this file extensively. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#17380 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAecV4j9dGKNQdH_n-C_p-THmbJyiHQGks5s7fxagaJpZM4QuNUb> .

refack · 2017-11-30T01:24:07Z

Since solving the debug release issue might take a while, i've refactored the deps patterns into /deps/.gitignore, so we can have something in the meanwhile.

MylesBorins · 2017-11-30T02:22:23Z

Thanks @refack Feel free to dismiss review as I'm not at my computer

…

On Nov 30, 2017 10:24 AM, "Refael Ackermann" ***@***.***> wrote: Since solving the debug release issue might take a while, i've refactored the deps patterns into /deps/.gitignore, so we can have something in the meanwhile. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#17380 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAecV12Q5CN9C01WV7KdnLK2Sge4zGxpks5s7gO8gaJpZM4QuNUb> .

MylesBorins · 2017-11-30T09:43:51Z

deps/.gitignore

+/openssl/openssl.targets
+/openssl/openssl.xml
+
+!/npm/node_modules**


should this be !/npm?

the other files in /deps/npm aren't ignores by any other rule, only node_modules is. So this is the minimal glob needed.

MylesBorins · 2017-11-30T09:44:23Z

.gitignore

-!test/fixtures/**/.*
-!tools/eslint/**/.*
-!tools/doc/node_modules/**/.*
+!deps/**


should it at the very least be at the bottom to avoid other rules combatting it?

Not longer needs to block, not ready to sign off just yet

gibfahn · 2017-11-30T14:17:12Z

It's the classical single-point-of-change vs. modularity issue...
My main concern is that git still lacks good exclusion traceability, i.e. it's not easy to correlate an actual exclusion with a .gitignore rule. So tracking them down in multiple files will increase that challenge.

FWIW I'd rather we have a single file. Yes it might look complex, but you don't have to worry about how rules in different files affect each other.

MylesBorins · 2017-12-04T20:18:06Z

Just hit an edge case on my machine

I gitignore global node_modules/... which in turn can trump our .gitignore in the project. Is there a way using .gitignore in the project to ensure that we trump rules in globalgitignore?

refack · 2017-12-04T20:49:02Z

According to the git manual our .gitignore should trump core.excludesfile - https://git-scm.com/docs/gitignore, so I'd assume the patterns in this PR should work (starts with ignoring all node_modules then explicitly exclude the ones we track)

BTW: I've found the command to trace an ignored file to it's exclude rule - git check-ignore -v --no-index THE_IGNORED_FILENAME

BridgeAR · 2018-01-31T02:44:17Z

This needs a rebase @refack

BridgeAR · 2018-02-07T03:44:50Z

Ping @refack again

BridgeAR · 2018-02-16T23:58:04Z

I am closing this due to the long inactivity. @refack please feel free to reopen in case you would like to work on this again!

tools,build: .gitignore tweaks

a84f58d

nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label Nov 28, 2017

MylesBorins reviewed Nov 29, 2017

View reviewed changes

gibfahn reviewed Nov 29, 2017

View reviewed changes

MylesBorins previously requested changes Nov 30, 2017

View reviewed changes

refack added the wip Issues and PRs that are still a work in progress. label Nov 30, 2017

[fixup] refactor sub patterns to deps

e7cde42

[fixup] moar tweaks

869b204

refack removed the wip Issues and PRs that are still a work in progress. label Nov 30, 2017

MylesBorins reviewed Nov 30, 2017

View reviewed changes

MylesBorins mentioned this pull request Nov 30, 2017

tools: whitelist npm directory in .gitignore #17363

Closed

MylesBorins force-pushed the master branch from b7405ab to 7f086dd Compare December 8, 2017 16:37

maclover7 force-pushed the master branch from bb5575a to 993b716 Compare January 26, 2018 22:02

cjihrig force-pushed the master branch from 993b716 to 082f952 Compare January 26, 2018 22:36

BridgeAR closed this Feb 16, 2018

refack deleted the git-ignore-tweaks branch September 5, 2018 22:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tools,build: .gitignore tweaks #17380

tools,build: .gitignore tweaks #17380

refack commented Nov 28, 2017 •

edited

Loading

MylesBorins left a comment

MylesBorins Nov 29, 2017

refack Nov 29, 2017

MylesBorins Nov 30, 2017

refack Nov 30, 2017

gibfahn Nov 29, 2017

refack Nov 29, 2017

MylesBorins Nov 29, 2017

refack Nov 29, 2017

MylesBorins commented Nov 29, 2017 •

edited

Loading

gibfahn Nov 29, 2017

refack commented Nov 29, 2017

MylesBorins commented Nov 29, 2017

MylesBorins left a comment

refack commented Nov 30, 2017

MylesBorins commented Nov 30, 2017 via email

refack commented Nov 30, 2017

MylesBorins commented Nov 30, 2017 via email

MylesBorins Nov 30, 2017

refack Nov 30, 2017

MylesBorins Nov 30, 2017

gibfahn commented Nov 30, 2017

MylesBorins commented Dec 4, 2017

refack commented Dec 4, 2017

BridgeAR commented Jan 31, 2018

BridgeAR commented Feb 7, 2018

BridgeAR commented Feb 16, 2018

	# npm stuff
	node_modules
	package-lock.json
	!/deps/npm/node_modules**
	/deps/npm/node_modules/node-gyp/gyp/*/.pyc
	!/tools/eslint/node_modules**
	!/tools/doc/node_modules**
	!/test/fixtures/**
	# not needed and causes issues for distro packagers
	/deps/npm/node_modules/.bin/

tools,build: .gitignore tweaks #17380

tools,build: .gitignore tweaks #17380

Conversation

refack commented Nov 28, 2017 • edited Loading

Before

After

Checklist

Affected core subsystem(s)

MylesBorins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MylesBorins commented Nov 29, 2017 • edited Loading

Choose a reason for hiding this comment

refack commented Nov 29, 2017

MylesBorins commented Nov 29, 2017

MylesBorins left a comment

Choose a reason for hiding this comment

refack commented Nov 30, 2017

MylesBorins commented Nov 30, 2017 via email

refack commented Nov 30, 2017

MylesBorins commented Nov 30, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibfahn commented Nov 30, 2017

MylesBorins commented Dec 4, 2017

refack commented Dec 4, 2017

BridgeAR commented Jan 31, 2018

BridgeAR commented Feb 7, 2018

BridgeAR commented Feb 16, 2018

refack commented Nov 28, 2017 •

edited

Loading

MylesBorins commented Nov 29, 2017 •

edited

Loading