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

build: remove --code-cache-path help option #28446

Closed

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Jun 27, 2019

This commit removes the now obsolete option.

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

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Jun 27, 2019
@danbev
Copy link
Contributor Author

danbev commented Jun 27, 2019

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, tough TBH I think we should just remove this option as it's obsolete

@danbev
Copy link
Contributor Author

danbev commented Jun 28, 2019

LGTM, tough TBH I think we should just remove this option as it's obsolete

That would be even better. Can I go ahead and remove it? Thanks

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.

LGTM as-is or with the option removed per @joyeecheung's comment and @danbev's response.

This commit removes the now obsolete option.
This commit renames node_code_cache_path and removes the path from as
it does not really describe the configuration property as a path
anymore. Instead it indicates whether the code cache is used or not.
@danbev danbev force-pushed the build_configure_code_cache_path_help branch from f3aac03 to 5dfb434 Compare July 2, 2019 05:58
@danbev danbev changed the title build: fix --code-cache-path help option build: remove --code-cache-path help option Jul 2, 2019
@nodejs-github-bot
Copy link
Collaborator

@danbev danbev requested a review from joyeecheung July 3, 2019 04:00
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 with a nit

configure.py Outdated Show resolved Hide resolved
@danbev
Copy link
Contributor Author

danbev commented Jul 4, 2019

Landed in 39a9358.

@danbev danbev closed this Jul 4, 2019
@danbev danbev deleted the build_configure_code_cache_path_help branch July 4, 2019 05:10
danbev added a commit that referenced this pull request Jul 4, 2019
This commit removes the now obsolete option.

PR-URL: #28446
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Jul 20, 2019
This commit removes the now obsolete option.

PR-URL: #28446
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
This was referenced Jul 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants