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

build: sort .PHONY rules and add test-gc-clean #12059

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@joyeecheung
Member

joyeecheung commented Mar 27, 2017

The first commit sorts the .PHONY rules for the ease of updating and backporting changes to this list.
The second commit adds a test-gc-clean rule to clean the files generated during make test-gc

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

build

@joyeecheung

This comment has been minimized.

Show comment
Hide comment
@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott
Member

Trott commented Mar 27, 2017

@joyeecheung

This comment has been minimized.

Show comment
Hide comment
@joyeecheung

joyeecheung Apr 1, 2017

Member

CI errors don't seem to be related..pinging @nodejs/build again

Member

joyeecheung commented Apr 1, 2017

CI errors don't seem to be related..pinging @nodejs/build again

@Trott

Trott approved these changes Apr 1, 2017

LGTM

@gibfahn

gibfahn approved these changes Apr 1, 2017

Should we be using test-gc-clean elsewhere in the Makefile (e.g. in clean)?

@joyeecheung

This comment has been minimized.

Show comment
Hide comment
@joyeecheung

joyeecheung Apr 1, 2017

Member

@gibfahn I am still trying to put together a make test-clean(or, an additional test-clean-all for tests that don't get run in a simple make test)..I think make clean should be the reverse of make, not make test.

Member

joyeecheung commented Apr 1, 2017

@gibfahn I am still trying to put together a make test-clean(or, an additional test-clean-all for tests that don't get run in a simple make test)..I think make clean should be the reverse of make, not make test.

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Apr 1, 2017

Member

@joyeecheung I think make clean should clean up everything and take you back to a pristine state (as if I'd cloned a fresh copy of the repo). At least for me, the aim is to have the Makefile be as intuitive as possible, so contributors have to delve into its rules as little as possible.

If you say "I want to clean up", you probably want to clean up everything (make clean -> a clean directory). If you say "I only want to clean up X", then you'd probably expect a make clean-X. The latter case is probably rare, how often do you only want to clean up one thing?

Cleaning up more than you expected is annoying (you have to rebuild everything). Cleaning up less than you expected can be confusing (why are my gc tests still passing even though I actually broke native modules).

Also, make is a shortcut for make all, so to be consistent you'd have to have a make clean-all, which would be way too confusing (it would only clean up build artefacts).


If you wanted to add a make build-clean or something then I guess that might be useful, although if you're wiping the build you almost certainly want to regenerate the test artifacts.

I see the reason for having a make test-clean, but not for having a separate make test-clean-all. In what scenario do you not want to clean up all test artifacts? I don't think there's much overhead to always trying to clean gc (for example) even if it's not there.

Member

gibfahn commented Apr 1, 2017

@joyeecheung I think make clean should clean up everything and take you back to a pristine state (as if I'd cloned a fresh copy of the repo). At least for me, the aim is to have the Makefile be as intuitive as possible, so contributors have to delve into its rules as little as possible.

If you say "I want to clean up", you probably want to clean up everything (make clean -> a clean directory). If you say "I only want to clean up X", then you'd probably expect a make clean-X. The latter case is probably rare, how often do you only want to clean up one thing?

Cleaning up more than you expected is annoying (you have to rebuild everything). Cleaning up less than you expected can be confusing (why are my gc tests still passing even though I actually broke native modules).

Also, make is a shortcut for make all, so to be consistent you'd have to have a make clean-all, which would be way too confusing (it would only clean up build artefacts).


If you wanted to add a make build-clean or something then I guess that might be useful, although if you're wiping the build you almost certainly want to regenerate the test artifacts.

I see the reason for having a make test-clean, but not for having a separate make test-clean-all. In what scenario do you not want to clean up all test artifacts? I don't think there's much overhead to always trying to clean gc (for example) even if it's not there.

@joyeecheung

This comment has been minimized.

Show comment
Hide comment
@joyeecheung

joyeecheung Apr 1, 2017

Member

@gibfahn Yeah I am convinced make clean should bring the repo back to a clean slate now.

As for make test-clean-all, that's part of the "bring it back to a clean slate" thing, since the artifact generations are scattered around in different rules, I think it would be nice to have a rule showing all the possible artifacts / non-standard tests. It's not really meant to be run directly...

Member

joyeecheung commented Apr 1, 2017

@gibfahn Yeah I am convinced make clean should bring the repo back to a clean slate now.

As for make test-clean-all, that's part of the "bring it back to a clean slate" thing, since the artifact generations are scattered around in different rules, I think it would be nice to have a rule showing all the possible artifacts / non-standard tests. It's not really meant to be run directly...

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Apr 2, 2017

Member

As for make test-clean-all, that's part of the "bring it back to a clean slate" thing, since the artifact generations are scattered around in different rules, I think it would be nice to have a rule showing all the possible artifacts / non-standard tests. It's not really meant to be run directly...

In my mind you'd have:

clean: clean-test clean-build [...?]

clean-test: clean-test-gc clean-test-addons [...]

So clean-test would do what I think you mean by clean-test-all. You might have a clean-test-fixtures or something, which would just clean up the artifacts from the default test suites.

Member

gibfahn commented Apr 2, 2017

As for make test-clean-all, that's part of the "bring it back to a clean slate" thing, since the artifact generations are scattered around in different rules, I think it would be nice to have a rule showing all the possible artifacts / non-standard tests. It's not really meant to be run directly...

In my mind you'd have:

clean: clean-test clean-build [...?]

clean-test: clean-test-gc clean-test-addons [...]

So clean-test would do what I think you mean by clean-test-all. You might have a clean-test-fixtures or something, which would just clean up the artifacts from the default test suites.

@joyeecheung

This comment has been minimized.

Show comment
Hide comment
@joyeecheung

joyeecheung Apr 2, 2017

Member

@gibfahn What I had in mind is:

clean: test-clean-all clean-build ...
test-clean-all: test-clean test-addons-clean test-gc-clean
test-clean: # clean up stuff generated during make test

Anyways, one more CI for this PR...https://ci.nodejs.org/job/node-test-pull-request/7164/

Member

joyeecheung commented Apr 2, 2017

@gibfahn What I had in mind is:

clean: test-clean-all clean-build ...
test-clean-all: test-clean test-addons-clean test-gc-clean
test-clean: # clean up stuff generated during make test

Anyways, one more CI for this PR...https://ci.nodejs.org/job/node-test-pull-request/7164/

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 4, 2017

Member

This needs a rebase before it can land

Member

jasnell commented Apr 4, 2017

This needs a rebase before it can land

joyeecheung added some commits Mar 27, 2017

build: sort phony rules
Sort phony rules and place them one per line for the ease of
updating and backporting
@joyeecheung

This comment has been minimized.

Show comment
Hide comment
@joyeecheung

This comment has been minimized.

Show comment
Hide comment
@joyeecheung

joyeecheung Apr 11, 2017

Member

CI failures look unrelated. Landed in 9decfb1...baa2602, thanks!

Member

joyeecheung commented Apr 11, 2017

CI failures look unrelated. Landed in 9decfb1...baa2602, thanks!

joyeecheung added a commit that referenced this pull request Apr 11, 2017

build: sort phony rules
Sort phony rules and place them one per line for the ease of
updating and backporting

PR-URL: #12059
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

joyeecheung added a commit that referenced this pull request Apr 11, 2017

build: add test-gc-clean and test-gc PHONY rules
PR-URL: #12059
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@evanlucas

This comment has been minimized.

Show comment
Hide comment
@evanlucas

evanlucas Apr 25, 2017

Member

This is not landing cleanly on v7.x-staging. Mind submitting a backport?

Member

evanlucas commented Apr 25, 2017

This is not landing cleanly on v7.x-staging. Mind submitting a backport?

@jasnell jasnell referenced this pull request May 11, 2017

Closed

8.0.0 Release Proposal #12220

@gibfahn gibfahn referenced this pull request Jun 15, 2017

Closed

Auditing for 6.11.1 #230

2 of 3 tasks complete
@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Jun 18, 2017

Member

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

Member

gibfahn commented Jun 18, 2017

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

@joyeecheung joyeecheung self-assigned this Oct 17, 2017

@joyeecheung joyeecheung removed their assignment Jan 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment