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

The version list should be sorted by name #45630

Closed
MrJithil opened this issue Nov 26, 2022 · 8 comments · Fixed by #46428
Closed

The version list should be sorted by name #45630

MrJithil opened this issue Nov 26, 2022 · 8 comments · Fixed by #46428

Comments

@MrJithil
Copy link
Member

MrJithil commented Nov 26, 2022

The version list should be sorted.

Current Behaviour:

process.versions
{
  node: '20.0.0-pre',
  v8: '10.8.168.20-node.8',
  uv: '1.44.2',
  zlib: '1.2.13',
  brotli: '1.0.9',
  ares: '1.18.1',
  modules: '111',
  nghttp2: '1.51.0',
  napi: '8',
  llhttp: '8.1.0',
  cjs_module_lexer: '1.2.2',
  base64: '0.5.0',
  openssl: '3.0.7+quic',
  cldr: '42.0',
  icu: '72.1',
  tz: '2022f',
  unicode: '15.0',
  ngtcp2: '0.8.1',
  nghttp3: '0.7.0'
}

Problem with the current order:

Since the current version list is not sorted, it's hard to compare with the source code (mainly the deps folder).

Solution:

Except the node, sort all other versions by name.
Eg:

process.versions
{
  node: '20.0.0-pre',
  ares: '1.18.1',
  base64: '0.5.0',
  brotli: '1.0.9',
  cjs_module_lexer: '1.2.2',
  cldr: '42.0',
  ...
  ...
  uv: '1.44.2',
  v8: '10.8.168.20-node.8',
  zlib: '1.2.13',
}
@MrJithil
Copy link
Member Author

MrJithil commented Nov 26, 2022

Any easy ways to sort this macro in cpp?

#define NODE_VERSIONS_KEYS(V)                                                  \
  NODE_VERSIONS_KEYS_BASE(V)                                                   \
  NODE_VERSIONS_KEY_CRYPTO(V)                                                  \
  NODE_VERSIONS_KEY_INTL(V)                                                    \
  NODE_VERSIONS_KEY_QUIC(V)

@mscdex
Copy link
Contributor

mscdex commented Nov 26, 2022

Object.keys(process.versions).sort().reduce((r, k) => (r[k] = process.versions[k], r), {});

or

Object.fromEntries(Object.entries(process.versions).sort((a,b)=>a<b?-1:1))

@MrJithil
Copy link
Member Author

MrJithil commented Nov 27, 2022

```js
Object.keys(process.versions).sort().reduce((r, k) => (r[k] = process.versions[k], r), {});

or

Object.fromEntries(Object.entries(process.versions).sort((a,b)=>a<b?-1:1))

Sorry. This is not JS. I was in an expectation like, #define will be sufficient for a programmers to understand that it's a C++ code.

Editing the above comment.

@bnoordhuis
Copy link
Member

Any easy ways to sort this macro in cpp?

No, unfortunately. What you could do instead is:

  1. write out the READONLY_STRING_PROPERTY calls in src/node_process_object.cc by hand, or

  2. store the keys and values in an array and qsort/std::sort at runtime, or

  3. write a j2sc-like script that spits out code that is then incorporated in the final build

(1) is kind of fragile and a maintenance drag. (2) feels meh. (3) increases total build time, possibly by a lot.

A Sufficiently Smart Compiler would do the sorting in (2) at compile time. Unlikely, though.

The runtime version string munging in src/node_metata.cc is pretty inefficient. Likely doesn't matter because we usually deserialize from snapshot but if it ran every time, that'd be a good argument for going with (3).

@Neustradamus
Copy link

@MrJithil: Good point!

@himself65
Copy link
Member

#define V(key) \
if (!per_process::metadata.versions.key.empty()) { \
READONLY_STRING_PROPERTY( \
versions, #key, per_process::metadata.versions.key); \
}
NODE_VERSIONS_KEYS(V)
#undef V

@himself65
Copy link
Member

I think we could do here before it becomes a read-only object

Subject: [PATCH] src: sort versions
---
Index: src/node_process_object.cc
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/node_process_object.cc b/src/node_process_object.cc
--- a/src/node_process_object.cc	(revision 2a29df64645a70bbb833298423a29206c4ec6a2e)
+++ b/src/node_process_object.cc	(date 1675098681432)
@@ -107,7 +107,6 @@
 
   // process.versions
   Local<Object> versions = Object::New(isolate);
-  READONLY_PROPERTY(process, "versions", versions);
 
 #define V(key)                                                                 \
   if (!per_process::metadata.versions.key.empty()) {                           \
@@ -116,6 +115,9 @@
   }
   NODE_VERSIONS_KEYS(V)
 #undef V
+  // todo: sort the keys except node
+
+  READONLY_PROPERTY(process, "versions", versions);
 
   // process.arch
   READONLY_STRING_PROPERTY(process, "arch", per_process::metadata.arch);

@himself65
Copy link
Member

See #46428

nodejs-github-bot pushed a commit that referenced this issue Feb 18, 2023
Co-authored-by: James M Snell <[email protected]>
Co-authored-by: Anna Henningsen <[email protected]>
PR-URL: #46428
Fixes: #45630
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
MylesBorins pushed a commit that referenced this issue Feb 18, 2023
Co-authored-by: James M Snell <[email protected]>
Co-authored-by: Anna Henningsen <[email protected]>
PR-URL: #46428
Fixes: #45630
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
MylesBorins pushed a commit that referenced this issue Feb 20, 2023
Co-authored-by: James M Snell <[email protected]>
Co-authored-by: Anna Henningsen <[email protected]>
PR-URL: #46428
Fixes: #45630
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
anonrig pushed a commit to anonrig/node that referenced this issue Apr 6, 2023
Co-authored-by: James M Snell <[email protected]>
Co-authored-by: Anna Henningsen <[email protected]>
PR-URL: nodejs#46428
Fixes: nodejs#45630
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
danielleadams pushed a commit that referenced this issue Apr 11, 2023
Co-authored-by: James M Snell <[email protected]>
Co-authored-by: Anna Henningsen <[email protected]>
PR-URL: #46428
Fixes: #45630
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
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

Successfully merging a pull request may close this issue.

5 participants