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

[v9.x backport] build: add node_lib_target_name to cctest deps #18954

Closed
wants to merge 1 commit into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Feb 23, 2018

Currently the cctest target depend on the node_core_target_name
target. But it is the node_lib_target_name target that compiles the
sources now which means that if a source file in src is updated the
cctest executable will not be re-linked against it, but will remain
unchanged. The code will still be compiled, just not linked which
means that if you are debugging you'll not see the changes and also a
warning will be displayed about this issue.

This commit changes the cctest target to depend on node_lib_target_name.

PR-URL: #18576
Reviewed-By: Matheus Marchini matheus@sthima.com
Reviewed-By: Yihong Wang yh.wang@ibm.com
Reviewed-By: Gibson Fahnestock gibfahn@gmail.com
Reviewed-By: Ben Noordhuis info@bnoordhuis.nl

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

build

Currently the cctest target depend on the node_core_target_name
target. But it is the node_lib_target_name target that compiles the
sources now which means that if a source file in src is updated the
cctest executable will not be re-linked against it, but will remain
unchanged. The code will still be compiled, just not linked which
means that if you are debugging you'll not see the changes and also a
warning will be displayed about this issue.

This commit changes the cctest target to depend on node_lib_target_name.

PR-URL: nodejs#18576
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Reviewed-By: Yihong Wang <yh.wang@ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. v9.x labels Feb 23, 2018
@danbev
Copy link
Contributor Author

danbev commented Feb 23, 2018

@danbev danbev changed the title build: add node_lib_target_name to cctest deps [v9.x backport] build: add node_lib_target_name to cctest deps Feb 23, 2018
@mmarchini
Copy link
Contributor

@danbev this will break once #18550 lands. Should I add a commit to #18550 with the same fix you did on #18576?

@danbev
Copy link
Contributor Author

danbev commented Feb 26, 2018

Should I add a commit to #18550 with the same fix you did on #18576?

That sounds great! Let me know if we can close this in that case. Thanks

@mmarchini
Copy link
Contributor

Commit added to #18550. @danbev I think we can close this now (no need to resolve the conflict since I'll fix it on #18550)

@danbev
Copy link
Contributor Author

danbev commented Feb 26, 2018

@mmarchini Great, thanks for that. Will close now.

@danbev danbev closed this Feb 26, 2018
@danbev danbev deleted the backport-18576-9.x branch March 27, 2018 06:15
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.

3 participants