shared_library builds have moved location #2233
Comments
|
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 |
|
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 a2ed0df I can't really make sense of the original behaviour. And this comment prefixing it all just makes it even more confusing: Combined with the But IMO Nate baked in the behaviour for node-gyp that it should be in the top of 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? |
|
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 |
|
@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. |
|
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. |
|
OK, here's why we need to fix this: MylesBorins/node@73150f9#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. |
|
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 |
|
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. |
|
The Lines 7 to 8 in 1e2e94d Lines 41 to 43 in 1e2e94d which means it doesn't get follow the same path as node-gyp/gyp/pylib/gyp/generator/make.py Lines 2213 to 2223 in 3baa4e4 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? |
FWIW Changing that to `7.x` gives:
|
|
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. |
|
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) |
Ref: nodejs/node-gyp#2233 Ref: nodejs/gyp-next#69 PR-URL: #35635 Refs: https://github.com/nodejs/gyp-next/releases/tag/v0.6.0 Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>
Ref: nodejs/node-gyp#2233 Ref: nodejs/gyp-next#69 PR-URL: #35635 Refs: https://github.com/nodejs/gyp-next/releases/tag/v0.6.0 Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>
Ref: nodejs/node-gyp#2233 Ref: nodejs/gyp-next#69 PR-URL: nodejs#35635 Refs: https://github.com/nodejs/gyp-next/releases/tag/v0.6.0 Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>
… 0.6.0 release - refs nodejs/node-gyp#2233
… 0.6.0 release - refs nodejs/node-gyp#2233
There's some urgency to this with the pending npm@7 release.
Ref: nodejs/node#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
The text was updated successfully, but these errors were encountered: