Skip to content

NODE_MODULE_VERSION debian/sid 8 and 10 #621

Closed
Closed
NODE_MODULE_VERSION debian/sid 8 and 10#621

Description

@kapouer
kapouer
opened on Nov 2, 2018

Currently debian/sid ships openssl 1.1.1, and since it favors building nodejs using the shared library,
the NODE_MODULE_VERSION needs to be different for both Node 8 and 10.

Note that this is made possible by work done at nodejs/node#18770.

So i'd like to reserve these for debian:

For Node 8
NODE_MODULE_VERSION 58

For Node 10
NODE_MODULE_VERSION 65

Thanks !

Activity

  1. Trott commented on Nov 2, 2018

    @Trott
    Trott
    on Nov 2, 2018
    Member

    I don't think the TSC is the right repo/group for this request, but I'm not sure what is. The main repo? A release/LTS repo? Something else? @nodejs/tsc

  2. eli-schwartz commented on Nov 2, 2018

    @eli-schwartz
    eli-schwartz
    on Nov 2, 2018

    This is definitely the right repo for this request, via nodejs/node#22237 which added this documentation blurb: https://github.com/nodejs/node/blob/master/BUILDING.md#note-for-downstream-distributors-of-nodejs

  3. MylesBorins commented on Nov 2, 2018

    @MylesBorins
    MylesBorins
    on Nov 2, 2018
    Contributor

    /cc @ofrobots who had a good idea for a module numbering scheme

    I think the idea was for different organizations to be able to select a most significant bit so we could keep module version number sequential for core

  4. ofrobots commented on Nov 2, 2018

    @ofrobots
    ofrobots
    on Nov 2, 2018
    Contributor

    Yeah, I was thinking of using the most-significant 8-bits to encode a vendor. Something along the lines of this patch:

    diff --git a/src/node_version.h b/src/node_version.h
index d542193332..747cb16bf8 100644
--- a/src/node_version.h
+++ b/src/node_version.h
@@ -114,7 +114,18 @@
  *
  * More information can be found at https://nodejs.org/en/download/releases/
  */
-#define NODE_MODULE_VERSION 67
+#define NODE_MODULE_VERSION_BASE 67
+
+#define NODE_MODULE_VENDOR_OFFICIAL 0
+#define NODE_MODULE_VENDOR_ELECTRON 1
+#define NODE_MODULE_VENDOR_DEBIAN   2
+// More vendors can be added here.
+#define NODE_MODULE_VENDOR_UNKNOWN  255
+
+#define NODE_MODULE_VENDOR NODE_MODULE_VENDOR_UNKNOWN
+
+#define NODE_MODULE_VERSION ((NODE_MODULE_VENDOR << 24) | NODE_MODULE_VERSION_BASE)
+

 // the NAPI_VERSION provided by this version of the runtime
 #define NAPI_VERSION  3

  5. nicolasnoble commented on Nov 2, 2018

    @nicolasnoble
    nicolasnoble
    on Nov 2, 2018

    I'd like to mention that @ofrobots's suggestion means that even if it's adopted now, it can be retroactively and transparently applied to all current versions of the nodejs runtime. As such, Debian's assigned runtime numbers would be 0x02000039 for node 8 (or 33554489 in base 10), and 0x02000040 (or 33554496 in base 10) for node 10.

  6. kapouer commented on Nov 3, 2018

    @kapouer
    kapouer
    on Nov 3, 2018
    Author

    Isn't that going to make a somewhat weird shared library name ? libnode33554489.so ?
    edit it would be libnode.so.33554489

  7. Flarna commented on Nov 3, 2018

    @Flarna
    Flarna
    on Nov 3, 2018
    Member

    Wouldn't this lead to a needless explosion of binaries to build/maintain for native addons shipping pre-compiled binaries?

    Most addons use only the v8 APIs therefore difference in OpenSSL,... is no issue for them. But there is no possibility to detect this based on the NODE_MODULE_VERSION.
    Native addons are loaded from Javascript where version of all third parties are available. By using this addons which use more then the v8 API can add a dimension to select the right binary.

    Similar for addons which are always compiled on NPM install I think it makes more sense to offer specific defines for the third party to select the right API if needed.

    For sure it would be nice to allow node to verify compatibility of addons not just based on NODE_MODULE_VERSION, maybe an addon could specify during register the API versions it uses from all third parties?

    Edit: I assume it's not planned that debian,... also ships precompiled binaries for NPM modules which do this for official node variant.

  8. kapouer commented on Nov 3, 2018

    @kapouer
    kapouer
    on Nov 3, 2018
    Author

    @Flarna you assume right: the "correct" behavior for debian users is to either install a debian package (of a node addon) or build it from source - the only two viable options from a debian user perspective.

    (besides, you're very much right about registering/checking API versions a module uses).

  9. rvagg commented on Nov 6, 2018

    @rvagg
    rvagg
    on Nov 6, 2018
    Member

    How about we just add a docs/node_module_versions.md doc with a record of what's using what number, then our release docs can be updated to say that you should check that doc to find the "next" number for Node.

    This might be helpful to have centrally cause it's getting a bit out of hand and it's useful to know for NAN and probably N-API too. Add-on authors can then cross-reference it for deciding what binaries they want to provide too.

  10. rvagg commented on Nov 6, 2018

    @rvagg
    rvagg
    on Nov 6, 2018
    Member

    Trying out the idea of a registry here: nodejs/node#24114, lemme know what you think.

  11. rvagg commented on Nov 6, 2018

    @rvagg
    rvagg
    on Nov 6, 2018
    Member

    Hey @kapouer, you'd better double-check whether 1.1.1 against Node 10 is going to be ABI breaking? It's not intended to be on our side and support should be merged into 10 when it's ready in nodejs/node#18770. Of course we need to double check on our end but the OpenSSL team have said it should be ABI and API compatible with 1.1.0 and that's the assumption we're working on too. 1.1.0 support ceases before 10 LTS so this better be the case!
    Perhaps you only need to reserve one for 8+1.1.1?

  12. kapouer commented on Nov 6, 2018

    @kapouer
    kapouer
    on Nov 6, 2018
    Author

    Yes it is only for nodejs 8+1.1.0. It might not even be needed if i manage to get node 10 in next debian release, so it's more a concern for derivatives right now (ubuntu ?)

  13. MylesBorins commented on Jan 9, 2019

    @MylesBorins
    MylesBorins
    on Jan 9, 2019
    Contributor

    What remains for us to do here?

  14. kapouer commented on Jan 9, 2019

    @kapouer
    kapouer
    on Jan 9, 2019
    Author

    IMO this has been addressed correctly. Distributors are now aware of the problem thanks to the documentation. I'm inclined to close this.

  15. MylesBorins commented on Jan 10, 2019

    @MylesBorins
    MylesBorins
    on Jan 10, 2019
    Contributor

    Closing

  16. MylesBorins
    closed this as completedon Jan 10, 2019
  17. rvagg
    added a commit that references this issue on Apr 17, 2019

    doc: add ABI version registry

    c61c722
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions