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

gyp: implement LINK for ninja #14227

Merged
merged 1 commit into from
Jul 17, 2017
Merged

Conversation

sam-github
Copy link
Contributor

LINK is used for FIPS, and needs to set both the ld = and ldxx =
variables in the ninja build file to link c++ (node) and c (openssl-cli,
etc.) executables.

Without this, ninja can't link node when buliding in FIPS mode, test using ./configure --ninja --openssl-fips=/home/sam/w/core/openssl-fips-2.0.9/canister (obviously, with your own FIPS canister).

Affected core subsystem(s)

crypto, deps, gyp

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory. labels Jul 13, 2017
@sam-github
Copy link
Contributor Author

Based on #14115 (comment), @bnoordhuis , PTAL

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

Oy

@refack
Copy link
Contributor

refack commented Jul 13, 2017

Feel like pushing it upstream (than I can approve and land it 😈).

@sam-github
Copy link
Contributor Author

@refack where is upstream?

@@ -1927,6 +1927,9 @@ def GenerateOutputForConfig(target_list, target_dicts, data, params,
if key == 'CXX.host':
cxx_host = os.path.join(build_to_root, value)
cxx_host_global_setting = value
if key == 'LINK':
ld = os.path.join(build_to_root, value)
ldxx = os.path.join(build_to_root, value)
Copy link
Member

Choose a reason for hiding this comment

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

No LINK.host?

@refack
Copy link
Contributor

refack commented Jul 13, 2017

@refack where is upstream?

https://gyp.gsrc.io/docs/Hacking.md

@refack
Copy link
Contributor

refack commented Jul 13, 2017

@sam-github could you post the ninja files before the patch and after (probably just the openSSL)

@sam-github
Copy link
Contributor Author

sam-github commented Jul 13, 2017

Without this patch, LINK is completely ignored by the ninja generator, which means that

node/configure

Line 989 in 73078d6

['LINK', fips_ld + ' <(openssl_fips)/bin/fipsld'],
does nothing to build.ninja:

ld = $cc                                                                         
ldxx = $cxx 

With this patch, the value of LINK in configure is used to set ld and ldxx in out/Release/build.ninja.

ld = /home/sam/w/core/node/deps/openssl/fips/fipsld /home/sam/w/core/openssl-fips-2.0.9/canister/bin/fipsld
ldxx = /home/sam/w/core/node/deps/openssl/fips/fipsld /home/sam/w/core/openssl-fips-2.0.9/canister/bin/fipsld

Initially, I modified configure itself to set LD in addition to LINK, except that resulted in only ld = being set in the build.ninja, when both ld and ldxx need to be set.

@refack
Copy link
Contributor

refack commented Jul 13, 2017

With this patch, the value of LINK in configure is used to set ld and ldxx in out/Release/build.ninja.

Isn't that too global? or is that the whole point?

@sam-github
Copy link
Contributor Author

That is the whole point, and it is exactly what is done for the normal Makefile commands.

Right now, before this patch:

  • LINK sets the global link commands for Makefiles
  • LINK is ignored for ninja
  • LD sets the global ld command for ninja
  • nothing at all sets the global ldxx command for ninja, so you are out of luck if you need to set it

I don't understand gyp, maybe its intended that ninja and Makefiles do not share the same gyp variable name to set the link command? If so, then either LDXX needs to be added as a supported gyp var (to set ldxx in ninja), or LD in gyp needs to set both ld and ldxx in ninja.

As someone who doesn't get this entire tottering pile of build tools, I'll take suggestions on what is right!

With this PR, we can build FIPS on Linux with ninja, without it or something like it, we can't.

@sam-github
Copy link
Contributor Author

So, @refack what do you think? LINK sets ld&ldxx? Or leave LINK only for Makefiles, and LD sets ld, and LDXX sets ldxx? Or LD sets ld&ldxx? Which is more gypy/ninjay?

@refack
Copy link
Contributor

refack commented Jul 14, 2017

  • LINK sets the global link commands for Makefiles
  • LINK is ignored for ninja
  • LD sets the global ld command for ninja
  • nothing at all sets the global ldxx command for ninja, so you are out of luck if you need to set it

IMO:

  • LINK is probably a Makefile term
  • I don't see a reason not to implement it for ninja anyway
  • the LDXX issue is an oversight
  • You "should" implement LDXX, LDXX.host and LINK.host and write a GYP test if we want to upstream it (as a node floating patch it's good enough as is)

@refack
Copy link
Contributor

refack commented Jul 14, 2017

Which is more gypy/ninjay?

AFAIK ninja has no thumb-rules. All the variables are defined by GYP (ld, cc, cxx etc) so adding borrowing another noun into the GYP.ninja vocabulary seems perfectly legit.

@sam-github
Copy link
Contributor Author

out of curiosity, what does the ".host" mean?

@sam-github
Copy link
Contributor Author

@refack PTAL

@refack
Copy link
Contributor

refack commented Jul 14, 2017

Also perfectly legit. Might be easier to upstream...

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

Reaffirm

@sam-github
Copy link
Contributor Author

@bnoordhuis LGTY?

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

what does the ".host" mean?

Host toolchain. Relevant when cross-compiling.

@refack
Copy link
Contributor

refack commented Jul 15, 2017

I'm not sure which side of the LINK/LD+LDXX this points to but I've found this: indutny/gyp.js#37 (comment)

The ability to set the link rule is used for FIPS, and needs to set
both the `ld =` and `ldxx =` variables in the ninja build file to link
c++ (node) and c (openssl-cli, etc.) executables.

URL: nodejs#14227
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@sam-github sam-github merged commit a6cec04 into nodejs:master Jul 17, 2017
@sam-github sam-github deleted the ninja-link-support branch July 17, 2017 16:41
@addaleax
Copy link
Member

For releasers, this was landed with bogus PR-URL metadata.

@addaleax addaleax mentioned this pull request Jul 18, 2017
addaleax pushed a commit that referenced this pull request Jul 18, 2017
The ability to set the link rule is used for FIPS, and needs to set
both the `ld =` and `ldxx =` variables in the ninja build file to link
c++ (node) and c (openssl-cli, etc.) executables.

URL: #14227
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@sam-github
Copy link
Contributor Author

@addaleax what is wrong with the metadata? a6cec04 looks right to me

@addaleax
Copy link
Member

@sam-github It’s just a typo, the PR-URL: is missing the PR- part :)

@sam-github
Copy link
Contributor Author

Oops, yes, I remember now that there was some kind of paste accident. Sorry.

refack pushed a commit to refack/node that referenced this pull request Jul 18, 2017
The ability to set the link rule is used for FIPS, and needs to set
both the `ld =` and `ldxx =` variables in the ninja build file to link
c++ (node) and c (openssl-cli, etc.) executables.

PR-URL: nodejs#14227
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@refack
Copy link
Contributor

refack commented Jul 18, 2017

Well I'm re-floating it for #12632 so there it is now fixed

Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
The ability to set the link rule is used for FIPS, and needs to set
both the `ld =` and `ldxx =` variables in the ninja build file to link
c++ (node) and c (openssl-cli, etc.) executables.

URL: #14227
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
The ability to set the link rule is used for FIPS, and needs to set
both the `ld =` and `ldxx =` variables in the ninja build file to link
c++ (node) and c (openssl-cli, etc.) executables.

URL: #14227
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
The ability to set the link rule is used for FIPS, and needs to set
both the `ld =` and `ldxx =` variables in the ninja build file to link
c++ (node) and c (openssl-cli, etc.) executables.

URL: #14227
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
refack pushed a commit to refack/node that referenced this pull request Aug 23, 2017
The ability to set the link rule is used for FIPS, and needs to set
both the `ld =` and `ldxx =` variables in the ninja build file to link
c++ (node) and c (openssl-cli, etc.) executables.

URL: nodejs#14227
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit to addaleax/ayo that referenced this pull request Aug 25, 2017
The ability to set the link rule is used for FIPS, and needs to set
both the `ld =` and `ldxx =` variables in the ninja build file to link
c++ (node) and c (openssl-cli, etc.) executables.

URL: nodejs/node#14227
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Aug 28, 2017
The ability to set the link rule is used for FIPS, and needs to set
both the `ld =` and `ldxx =` variables in the ninja build file to link
c++ (node) and c (openssl-cli, etc.) executables.

URL: nodejs/node#14227
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 3, 2017
The ability to set the link rule is used for FIPS, and needs to set
both the `ld =` and `ldxx =` variables in the ninja build file to link
c++ (node) and c (openssl-cli, etc.) executables.

URL: #14227
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 5, 2017
The ability to set the link rule is used for FIPS, and needs to set
both the `ld =` and `ldxx =` variables in the ninja build file to link
c++ (node) and c (openssl-cli, etc.) executables.

URL: #14227
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
rvagg pushed a commit to nodejs/node-gyp that referenced this pull request Aug 9, 2018
The ability to set the link rule is used for FIPS, and needs to set
both the `ld =` and `ldxx =` variables in the ninja build file to link
c++ (node) and c (openssl-cli, etc.) executables.

URL: nodejs/node#14227
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
rvagg pushed a commit to nodejs/node-gyp that referenced this pull request Aug 9, 2018
The ability to set the link rule is used for FIPS, and needs to set
both the `ld =` and `ldxx =` variables in the ninja build file to link
c++ (node) and c (openssl-cli, etc.) executables.

URL: nodejs/node#14227
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
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. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants