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

Jump to bottom

build: mac OBJ_DIR should point to obj.target #11857

Closed
wants to merge 1 commit into from
Closed

build: mac OBJ_DIR should point to obj.target #11857

wants to merge 1 commit into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Mar 15, 2017

I think there might be an issue with the value of OBJ_DIR when
using a "mac" os. The value is currently specified in common.gypi
which is included by node.gyp:

'OBJ_DIR': '<(PRODUCT_DIR)/obj',

In the generated Makefile (out/Makefile) the object output directory
is:

obj := $(builddir)/obj

And in the included node.target.mk we have the OBJS declared:

OBJS := \
         $(obj).target/$(TARGET)/src/async-wrap.o \
         $(obj).target/$(TARGET)/src/cares_wrap.o \

If OBJ_DIR is used in node.gyp to point to generated object files
on mac they will not be found.

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

build

@danbev
build: mac OBJ_DIR should point to obj.target
0fa0768 
I think there might be an issue with the value of OBJ_DIR when
using a "mac" os. The value is currently specified in common.gypi
which is included by node.gyp:
'OBJ_DIR': '<(PRODUCT_DIR)/obj',

In the generated Makefile (out/Makefile) the object output directory
is:
obj := $(builddir)/obj

And in the included node.target.mk we have the OBJS declared:
OBJS := \
         $(obj).target/$(TARGET)/src/async-wrap.o \
         $(obj).target/$(TARGET)/src/cares_wrap.o \

If OBJ_DIR is used in node.gyp to point to generated object files
on mac they will not be found.
@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Mar 15, 2017
@danbev
Copy link
Contributor Author

danbev commented Mar 15, 2017

CI: https://ci.nodejs.org/job/node-test-pull-request/6852/

@danbev danbev mentioned this pull request Mar 15, 2017
Closed
2 tasks
danbev added a commit to danbev/node that referenced this pull request Mar 17, 2017
@danbev
build: mac OBJ_DIR should point to obj.target
f0d4237 
I think there might be an issue with the value of OBJ_DIR when
using a "mac" os. The value is currently specified in common.gypi
which is included by node.gyp:
'OBJ_DIR': '<(PRODUCT_DIR)/obj',

In the generated Makefile (out/Makefile) the object output directory
is:
obj := $(builddir)/obj

And in the included node.target.mk we have the OBJS declared:
OBJS := \
         $(obj).target/$(TARGET)/src/async-wrap.o \
         $(obj).target/$(TARGET)/src/cares_wrap.o \

If OBJ_DIR is used in node.gyp to point to generated object files
on mac they will not be found.

PR-URL: nodejs#11857
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@danbev
Copy link
Contributor Author

danbev commented Mar 17, 2017

Landed in f0d4237

@danbev danbev closed this Mar 17, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Mar 20, 2017
@danbev @italoacasas
build: mac OBJ_DIR should point to obj.target
5244ee3 
I think there might be an issue with the value of OBJ_DIR when
using a "mac" os. The value is currently specified in common.gypi
which is included by node.gyp:
'OBJ_DIR': '<(PRODUCT_DIR)/obj',

In the generated Makefile (out/Makefile) the object output directory
is:
obj := $(builddir)/obj

And in the included node.target.mk we have the OBJS declared:
OBJS := \
         $(obj).target/$(TARGET)/src/async-wrap.o \
         $(obj).target/$(TARGET)/src/cares_wrap.o \

If OBJ_DIR is used in node.gyp to point to generated object files
on mac they will not be found.

PR-URL: nodejs#11857
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@italoacasas italoacasas mentioned this pull request Mar 20, 2017
Merged
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
@danbev @jungx098
build: mac OBJ_DIR should point to obj.target
249ecef 
I think there might be an issue with the value of OBJ_DIR when
using a "mac" os. The value is currently specified in common.gypi
which is included by node.gyp:
'OBJ_DIR': '<(PRODUCT_DIR)/obj',

In the generated Makefile (out/Makefile) the object output directory
is:
obj := $(builddir)/obj

And in the included node.target.mk we have the OBJS declared:
OBJS := \
         $(obj).target/$(TARGET)/src/async-wrap.o \
         $(obj).target/$(TARGET)/src/cares_wrap.o \

If OBJ_DIR is used in node.gyp to point to generated object files
on mac they will not be found.

PR-URL: nodejs#11857
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Member

should this land on v6.x?

@danbev danbev deleted the common.gypi-mac-OBJ_DIR branch April 20, 2017 07:52
@MylesBorins
Copy link
Member

ping

@danbev
Copy link
Contributor Author

danbev commented May 8, 2017

Sorry about missing this one. I'll take a look at this tomorrow.

@danbev
Copy link
Contributor Author

danbev commented May 9, 2017 •
edited by gibfahn

@MylesBorins Sorry about the delay on this. I don't think this is necessary to backport. It would not hurt to do so, but this was added when we were working on how to get the cctest to use object files. That is not done in the v6.x-staging branch which is why I think this one can be skipped.

@gibfahn
Copy link
Member

gibfahn commented May 9, 2017

So we'd want this once/if #11956 gets backported? @danbev I note that's backport-requested, any chance you could raise a backport for it?

@danbev
Copy link
Contributor Author

danbev commented May 9, 2017

@gibfahn Oh I was not aware of that. I'll take a look and raise a PR for that backport and a backport for this. @MylesBorins Ignore my earlier comment :)

@MylesBorins MylesBorins added the baking-for-lts PRs that need to wait before landing in a LTS release. label May 15, 2017
@refack refack added the embedding Issues and PRs related to embedding Node.js in another project. label Aug 17, 2017
@MylesBorins MylesBorins removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Aug 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@jasnell jasnell jasnell approved these changes

@cjihrig cjihrig cjihrig approved these changes

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. embedding Issues and PRs related to embedding Node.js in another project.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@danbev @MylesBorins @gibfahn @bnoordhuis @jasnell @cjihrig @refack @nodejs-github-bot