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.

Sign up for GitHub

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

Jump to bottom

doc: add note in BUILDING.md about running make distclean #31542

Closed
wants to merge 3 commits into from
Closed

doc: add note in BUILDING.md about running make distclean #31542

wants to merge 3 commits into from

Conversation

swagatata
Copy link
Contributor

@swagatata swagatata commented Jan 28, 2020 •
edited
Loading

Fixes: #28675

Checklist

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. labels Jan 28, 2020
mscdex
mscdex reviewed Jan 28, 2020
View reviewed changes
BUILDING.md Outdated Show resolved Hide resolved
@mscdex
Copy link
Contributor

mscdex commented Jan 28, 2020

May want to also mention the need to re-run the make command after make distclean.

@swagatata swagatata force-pushed the swagat-branch branch 2 times, most recently from 56e23fa to 2ca77d7 Compare January 29, 2020 00:50
mscdex
mscdex reviewed Jan 29, 2020
View reviewed changes
BUILDING.md Outdated Show resolved Hide resolved
Trott
Trott reviewed Jan 29, 2020
View reviewed changes
BUILDING.md Outdated Show resolved Hide resolved
@Trott
Copy link
Member

Trott commented Jan 29, 2020

Welcome @swagatata and thanks for the pull request. Small documentation pull requests can attract a lot of comments, so your patience is appreciated!

mhdawson
mhdawson approved these changes Jan 30, 2020
View reviewed changes
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 after suggestions.

@Trott

This comment has been minimized.

Sign in to view
@Trott
Copy link
Member

Trott commented Feb 1, 2020

I went ahead and applied the earlier nits via --fixup.. So, revising, here are the remaining suggestions:

  • I think this suggestion is in the wrong place. It appears in the build flow description so it looks like it's part of the happy path. It should be in a troubleshooting section instead, or at least at the very, very end of the section.

  • It should explain to people what make distclean does.

  • It should warn the user that after running make distclean, they will have to run ./configure again if they had any non-default options because make distclean removes config.mk (unlike make clean).

  • It should warn the user that after running make distclean, they will need to do a full build so it will take a long time.

  • This should probably document make clean as well, along with the difference between clean and distclean.

@swagatata swagatata force-pushed the swagat-branch branch 2 times, most recently from 984ed3d to 97741b7 Compare February 1, 2020 23:52
@swagatata
Copy link
Contributor Author

I have tried to follow @Trott 's suggestions, adding a "cleaning up the build" section and "troubleshooting" section.

I'm not very well versed with what exactly "make clean" does and how it differs from "make distclean" so I'd welcome some notes to add there.

Trott
Trott requested changes Feb 2, 2020
View reviewed changes
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.

Needs a little bit more editing. Just leaving this here so no one lands this without those edits.

Trott
Trott approved these changes Feb 2, 2020
View reviewed changes
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.

I moved stuff around, changed headers, but the basic gist and fundamental wording is all still there, I think. PTAL.

@swagatata
Copy link
Contributor Author

Who can merge this?

@Trott
Copy link
Member

Trott commented Feb 7, 2020

Landed in a733c18

@Trott Trott closed this Feb 7, 2020
Trott pushed a commit that referenced this pull request Feb 7, 2020
@swagatata @Trott
doc: add note in BUILDING.md about running make distclean
a733c18 
Fixes: #28675

PR-URL: #31542
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@swagatata swagatata deleted the swagat-branch branch February 15, 2020 18:36
codebytere pushed a commit that referenced this pull request Feb 17, 2020
@swagatata @codebytere
doc: add note in BUILDING.md about running make distclean
b61b85c 
Fixes: #28675

PR-URL: #31542
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@codebytere codebytere mentioned this pull request Feb 17, 2020
Merged
codebytere pushed a commit that referenced this pull request Mar 15, 2020
@swagatata @codebytere
doc: add note in BUILDING.md about running make distclean
eee0777 
Fixes: #28675

PR-URL: #31542
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this pull request Mar 17, 2020
@swagatata @codebytere
doc: add note in BUILDING.md about running make distclean
b5b0dc9 
Fixes: #28675

PR-URL: #31542
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@codebytere codebytere mentioned this pull request Mar 17, 2020
Merged
codebytere pushed a commit that referenced this pull request Mar 30, 2020
@swagatata @codebytere
doc: add note in BUILDING.md about running make distclean
e83af20 
Fixes: #28675

PR-URL: #31542
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
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
Reviewers

@mscdex mscdex mscdex left review comments

@jasnell jasnell jasnell approved these changes

@Trott Trott Trott approved these changes

@mhdawson mhdawson mhdawson approved these changes

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

add note in BUILDING.md about running make distclean
6 participants
@swagatata @mscdex @Trott @jasnell @mhdawson @nodejs-github-bot