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: add CONFIG_FLAGS to with-code-cache target #22207

Closed
wants to merge 1 commit into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Aug 9, 2018

This commit adds CONFIG_FLAGS to allow the with-code-cache target to be
used with a debug build. The motivation for this is to make it easier to
debug a build with the code cache enabled.

The suggested usage:

$ make BUILDTYPE=Debug with-code-cache

The BUILDTYPE option is not needed if ./configure was already
configured with --debug.

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 Aug 9, 2018
@danbev
Copy link
Contributor Author

danbev commented Aug 9, 2018

@Trott
Copy link
Member

Trott commented Aug 9, 2018

@nodejs/build-files

Makefile Outdated
.PHONY: with-code-cache
with-code-cache:
$(PYTHON) ./configure
$(PYTHON) ./configure $(debug_flag)
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps it would be better to make the ./configure invocations in this recipe take $(CONFIG_FLAGS) (like the other invocations of ./configure in this file) and then the ifeq statement a few lines up could append --debug to that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I was not aware of the CONFIG_FLAGS, I'll take a look. Thanks!

This commit adds CONFIG_FLAGS to allow the with-code-cache target to be
used with a debug build. The motivation for this is to make it easier to
debug a build with the code cache enabled.

The suggested usage:
$ make BUILDTYPE=Debug with-code-cache

The BUILDTYPE option is not needed if ./configure was already
configured with --debug.
@danbev danbev changed the title build: add debug flag to with-code-cache target build: add CONFIG_FLAGS to with-code-cache target Aug 10, 2018
@danbev
Copy link
Contributor Author

danbev commented Aug 10, 2018

Updated and rebased CI: https://ci.nodejs.org/job/node-test-pull-request/16333/

@maclover7 maclover7 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 11, 2018
@maclover7
Copy link
Contributor

(One failure on CI is due to a SmartOS 17 machine that has been having issues, this should be safe to land)

@maclover7
Copy link
Contributor

Landed in ec8f31d

@maclover7 maclover7 closed this Aug 12, 2018
maclover7 pushed a commit that referenced this pull request Aug 12, 2018
This commit adds CONFIG_FLAGS to allow the with-code-cache target to be
used with a debug build. The motivation for this is to make it easier to
debug a build with the code cache enabled.

The suggested usage:
$ make BUILDTYPE=Debug with-code-cache

The BUILDTYPE option is not needed if ./configure was already
configured with --debug.

PR-URL: #22207
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
@danbev danbev deleted the build_codecache_debug_flag branch August 12, 2018 04:58
targos pushed a commit that referenced this pull request Aug 12, 2018
This commit adds CONFIG_FLAGS to allow the with-code-cache target to be
used with a debug build. The motivation for this is to make it easier to
debug a build with the code cache enabled.

The suggested usage:
$ make BUILDTYPE=Debug with-code-cache

The BUILDTYPE option is not needed if ./configure was already
configured with --debug.

PR-URL: #22207
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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