Open rvagg opened 3 years ago
ryzokuken
commented
3 years ago So, I have two thoughts. On one hand, I think I'd be really happy if we figured out what the more appropriate behavior was, and switch to that, atleast in the longer run. One another, I think we should really fix this behavior for node-gyp right now, handle it properly in node core (since that doesn't affect the ecosystem) and only change it if needed in a major release.
rvagg
commented
3 years ago I'm inclined to think that the appropriate behaviour is what it's always done, which is to place the final .so in the same place as standard target builds. Getting it baked into a major release of npm is going to be painful for us and the ecosystem—we're either going to have to adapt to it or quickly push out a "fix" for it because we were wrong.
Re https://github.com/nodejs/node-gyp/commit/a2ed0df84e294a197e3eb36b8213db6d8116028c
I can't really make sense of the original behaviour. And this comment prefixing it all just makes it even more confusing:
# Xcode puts shared_library results into PRODUCT_DIR, and some gyp files
# rely on this. Emulate this behavior for mac.Combined with the self.flavor != 'mac' leaves me scratching my head. Why is it good enough for mac to put it in PRODUCT_DIR but ever other platform to go deeper into lib.X? There must be some original precedent that made that the appropriate choice but then when they added Xcode support it had a different behaviour and they decided to unify the make behaviour on mac with the Xcode version. But it was too baked in at the time to move every other platform?
But IMO Nate baked in the behaviour for node-gyp that it should be in the top of $(builddir), so changing it is our problem now.
Maybe it's simply that gyp wasn't originally designed to build exposed shared libraries and that shared libraries were just intermediate targets to build the final exposed targets, so shared libraries were somehow meant to be hidden behind all of the gyp cruft (kind of weird with shared libraries, but perhaps?). But with node-gyp we're allowing users to expose shared libraries so the use-case is different to gyp's original intent.
What are the implications of changing this for nodejs/node?
targos
commented
3 years ago I don't remember what the implications are. We need to try a new CI run to see the exact errors. My first thought is that it can be an issue for cross-compiled builds, because the output for target and host would overwrite themselves.
targos
commented
3 years ago @rvagg do you know any project / npm module that relies on shared libraries being output in a specific place and will be broken by npm 7? We haven't found one in CITGM.
rvagg
commented
3 years ago Yeah, good question. Here's some candidates (not exhaustive since there are projects that have sub-definitions in files not named binding.gyp): https://github.com/search?l=&q=%22shared_library%22+filename%3Abinding.gyp&type=code
appmetrics is in the list, I imagine that's quite popular and worth testing out.
rvagg
commented
3 years ago OK, here's why we need to fix this: https://github.com/MylesBorins/node/commit/73150f94d8edd595c6aff83db56ea883483c60e8#diff-0e3a10eceb23013e92432a2df0befd92
-module.exports.load(`${path.dirname(bindingPath)}/ping.so`);
+
+let pingSOPath = `${path.dirname(bindingPath)}/lib.target/ping.so`;
+
+if (common.isOSX) {
+ pingSOPath = `${path.dirname(bindingPath)}/ping.so`;
+}
+
+console.log('module.exports.load:', pingSOPath);
+module.exports.load(pingSOPath);we have divergent behaviour between the Xcode generator and the make generator, as per the note in GYP about this. So not only would users need to adapt to a new location for shared libraries, they need to branch on platform.
Now I don't know who this would break for, there are plenty of projects out there using shared_library but I don't know if many are actually loading them from Node rather than simply using them for intermediates in their build. So maybe the impact is low. But the platform discrepancy is enough of a problem on its own that we should fix this.
targos
commented
3 years ago I agree that it is worth fixing, but I'm not certain it should be done in gyp itself. Would it be possible to move the files from node-gyp? How does it work with the .node binary now?
rvagg
commented
3 years ago It just defers entirely to gyp. node-gyp's role is to simply serve as a JS wrapper to setup the environment and invoke it with the right arguments.
richardlau
commented
3 years ago The .node binary is a loadable module:
https://github.com/nodejs/node-gyp/blob/1e2e94d9851a1685d80d591262d0730681b5481d/addon.gypi#L7-L8
https://github.com/nodejs/node-gyp/blob/1e2e94d9851a1685d80d591262d0730681b5481d/addon.gypi#L41-L43
which means it doesn't get follow the same path as shared_libarary:
https://github.com/nodejs/node-gyp/blob/3baa4e4172c47052ee7e942cef542c1a7dea75aa/gyp/pylib/gyp/generator/make.py#L2213-L2223
In practical terms perhaps we could add a switch the toggle the behavior in gyp and then pass/trigger that when calling gyp from node-gyp?
richardlau
commented
3 years ago Yeah, good question. Here's some candidates (not exhaustive since there are projects that have sub-definitions in files not named binding.gyp): https://github.com/search?l=&q=%22shared_library%22+filename%3Abinding.gyp&type=code
appmetrics is in the list, I imagine that's quite popular and worth testing out.
FWIW appmetrics is broken by this but it currently depends on node-gyp 5 so it isn't immediately obvious when testing with the current v15.0.0-proposal (with npm 7):
https://github.com/RuntimeTools/appmetrics/blob/4862e6882e85b63ea22c55cb4b273f17ef4820cb/package.json#L10
rvagg
commented
3 years ago this seems to work for nodejs/node https://github.com/nodejs/node/compare/master...rvagg:rvagg/gyp-lib.target?expand=1
essentially winding back the use of lib.target for any top-level target & dependency for make.
good across the containered tests which include a shared library build https://ci.nodejs.org/job/node-test-commit-linux-containered/22729/
running a full suite @ https://ci.nodejs.org/job/node-test-commit/41218/, the ARM cross compiler might be a concern, but it seems that removing lib.target entirely for targets in gyp-next might be a practical and solve the problem here and still supporting nodejs/node.
I'm off for the day if someone else is inclined to pick this up. I'm not sure I want to be the one to push this through tbh, I have quite a bit on my plate at the moment so I'd appreciate whatever effort others can throw at this.
rvagg
commented
3 years ago had a problem with the Pi3s, I had to restart them all, clear out workspaces and have restarted the arm-fanned here https://ci.nodejs.org/job/node-test-commit-arm-fanned/16803/
(actually heading off now)
There's some urgency to this with the pending npm@7 release.
Ref: https://github.com/nodejs/node/pull/35474
node-gyp 5.1.0 (current npm version):
current node-gyp master:
Note the difference in location of
ping.so. The test is expecting it to be inbuild/${common.buildType}/ping.so.binding.gyp is @ https://github.com/nodejs/node/blob/master/test/addons/dlopen-ping-pong/binding.gyp
@targos
npm@7 is about to go out with our latest code and it's going to mean shared libraries are in a different location, arguably an internal build location that shouldn't be of concern to consumers (like obj.target). This is going to mean people using shared library builds need to add yet another location to search for their .so on top of the current list (see https://ghub.io/bindings for the full list so far).
IMO this is a bug and we should fix it. But we need to figure out how to get it addressed here for our needs but not break nodejs/node.
This is mostly a question for @nodejs/gyp people since I think the bulk of the work needs to be done there.
/cc @nodejs/node-gyp