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

GypPathToUniqueOutput from ninja.py #37

Closed
indutny opened this issue Mar 21, 2017 · 17 comments
Closed

GypPathToUniqueOutput from ninja.py #37

indutny opened this issue Mar 21, 2017 · 17 comments

Comments

@indutny
Copy link
Owner

indutny commented Mar 21, 2017

Original GYP puts compilation results to obj/${intPrefix} dir if they are of some specific types (which differ by platform).

I'm not sure how to fix it at this moment, but node uses this feature of GYP when linking to v8/openssl. We need to fix it somehow.

cc @refack maybe you have an idea?

@refack
Copy link
Contributor

refack commented Mar 21, 2017

Do you have a tree example? On which platform?
(I assume not Windows since I was able to link node.exe on Windows)

@indutny
Copy link
Owner Author

indutny commented Mar 21, 2017

Not windows, actually. https://github.com/nodejs/node/blob/940b5303bef7aee9b24214c62e4b6f182f23f82a/node.gyp#L386-L399

It is different on Mac and other Unixes. On Mac GYP puts everything into ./out/Release be it libv8_base.a or whatever, on unixes it will put .a files in obj/... folders. I assume that this is because GYP prevents name conflicts... but still, it appears to work without it at the moment.

@indutny
Copy link
Owner Author

indutny commented Mar 21, 2017

The problem is that node.gyp expects the .a file to be at different location on Linux and FreeBSD.

@indutny
Copy link
Owner Author

indutny commented Mar 22, 2017

@indutny
Copy link
Owner Author

indutny commented Mar 22, 2017

@evmar sorry to bring you in here without asking you first, I hope this is ok.

You have reviewed that original CL for placing GYP outputs in different dirs on different platforms. May I ask you to share with us what downsides may be in placing everything in ./out/Release? I suppose it comes down to multiple-toolset, but otherwise is there anything else to consider?

@indutny
Copy link
Owner Author

indutny commented Mar 22, 2017

Will placing outputs to unique paths for toolset !== 'target' be enough to mitigate this?

@refack
Copy link
Contributor

refack commented Mar 22, 2017 via email

@evmar
Copy link

evmar commented Mar 22, 2017

@nico is the gyp+mac wizard who can maybe explain this decision.

I don't really understand this node code, so this advice is vague, but:

  • I think the design of gyp is that build rules shouldn't care where static libraries are written (the actual path the library ends up at is meant to be an internal implementation detail).
  • I would think the proper place for library-specific logic would be on the library's build rule, not on the rule that depends on the library.
  • Chrome uses OpenSSL (well, BoringSSL, but that's a fork of OpenSSL) and it doesn't need this special casing. Do you know why?

@indutny
Copy link
Owner Author

indutny commented Mar 22, 2017

I absolutely agree with all points here. The reason why node uses it is because addons (read shared_library) need to access OpenSSL and V8 functions. Hence, node.gyp has whole-archive linker directives. These directives will make all symbols available in the final executable, however they need the path to the static library which is placed somewhere in either product or object dir.

Anyway, the question is more about whether putting everything with toolset == 'target' in ./out/Release is a bad pattern, and if it should be avoided?

@indutny
Copy link
Owner Author

indutny commented Mar 22, 2017

( @evmar thanks for coming here with answers! We appreciate it! )

@evmar
Copy link

evmar commented Mar 22, 2017

My vague recollection of the history is that we originally only worked on Linux and put the archives in the subdirectories because otherwise they'd need to be globally unique names across the project. And then for some Mac reason we needed to put the archives not in subdirectories, which meant users now must be careful to use globally unique names. So I think it'd be harmless to put them all in the top-level directory on all platforms. Just kinda ugly! But I think @nico knows better than me.

@indutny
Copy link
Owner Author

indutny commented Mar 22, 2017

This does not work for projects whose output is either static or shared library, though. I suppose that ideally top-level .gyp file results may be put to ./out/Release, while others may be put to obj subfolders.

@evmar
Copy link

evmar commented Mar 23, 2017

(Just another disclaimer that I haven't worked in this area for >5 years, but...) As I recall static_library/shared_library was considered an internal build detail, and there was some other target type (maybe loadable_module?) for variants of those that were considered build output and which would get predictable paths.

@indutny
Copy link
Owner Author

indutny commented Mar 23, 2017

Yeah, sounds good then. I was afraid of some unforeseen caveats that I haven't considered. Going to keep it as it is now, and land #38. Thank yoU!

@indutny indutny closed this as completed Mar 23, 2017
@nico
Copy link

nico commented Apr 17, 2017

Sorry, didn't see this. The history of gyp, in a nutshell, is that @evmar wrote the Ninja generator and it was so good that all the other platforms wanted to use it. So we made it work on other platforms, but we already had other build systems on the other platforms. To make a good case for switching from these other platforms to ninja, we tried to make gyp's ninja generator produce bit-for-bit identical output as the "native" build system on these platforms. So on Mac, gyp/ninja produces ninja files that behave like Xcode (3.1.2) did, and on Win, it produces ninja files that behave like MSVC. That's the only reason for it.

@indutny
Copy link
Owner Author

indutny commented Apr 17, 2017

Thank you for informative reply!

@refack
Copy link
Contributor

refack commented Apr 17, 2017

Ref: nodejs/node#12231 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants