Skip to content

src: move arch, platform and release into node_metadata.cc - #25293

Closed
joyeecheung wants to merge 2 commits into
nodejs:masterfrom
joyeecheung:move-more-metadata
Closed

src: move arch, platform and release into node_metadata.cc#25293
joyeecheung wants to merge 2 commits into
nodejs:masterfrom
joyeecheung:move-more-metadata

Conversation

@joyeecheung

@joyeecheungjoyeecheung commented Dec 31, 2018•
edited
Loading

Copy link
Copy Markdown
Member

Move definitions of more metadata into node_metadata{.h, .cc}
so the data can be reused and easily inspected in C++.

In a peudo-release build:

(lldb) p node::per_process::metadata
(node::Metadata) $0 = {
versions = {
node = "12.0.0-nightly2018-12-31819a868a49a2b7dbb7b3adab2b79cc8b783f7279"
v8 = "7.1.302.33-node.8"
uv = "1.24.1"
zlib = "1.2.11"
ares = "1.15.0"
modules = "68"
nghttp2 = "1.34.0"
napi = "3"
llhttp = "1.0.1"
http_parser = "2.8.0"
openssl = "1.1.0j"
cldr = "34.0"
icu = "63.1"
tz = "2018e"
unicode = "11.0"
}
release = (name = "node", sourceUrl = "https://nodejs.org/download/release/v12.0.0-nightly2018-12-31819a868a49a2b7dbb7b3adab2b79cc8b783f7279/node-v12.0.0-nightly2018-12-31819a868a49a2b7dbb7b3adab2b79cc8b783f7279.tar.gz", headersUrl = "https://nodejs.org/download/release/v12.0.0-nightly2018-12-31819a868a49a2b7dbb7b3adab2b79cc8b783f7279/node-v12.0.0-nightly2018-12-31819a868a49a2b7dbb7b3adab2b79cc8b783f7279-headers.tar.gz")
arch = "x64"
platform = "darwin"
}
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@joyeecheung
src: move arch, platform and release into node_metadata.cc
819a868
Move definitions of more metadata into node_metadata{.h, .cc}
so the data can be reused and easily inspected in C++.
@nodejs-github-bot

nodejs-github-bot commented Dec 31, 2018

Copy link
Copy Markdown
Collaborator

@joyeecheung build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2117/pipeline

@nodejs-github-botnodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Dec 31, 2018
@joyeecheung

joyeecheung commented Dec 31, 2018

Copy link
Copy Markdown
MemberAuthor

CI: https://ci.nodejs.org/job/node-test-pull-request/19895/

addaleax
addaleax approved these changes Jan 1, 2019
View reviewed changes
Comment threadsrc/node_metadata.h Outdated
Release release;

std::string arch;
std::string platform;

@addaleaxaddaleaxJan 1, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the extern Metadata metadata; on L104 be const?

@joyeecheungjoyeecheungJan 4, 2019

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think Metadata itself can be const - the ICU data versions have to be loaded after they are initialized. Other members should be able to be made const though.

bnoordhuis
bnoordhuis reviewed Jan 3, 2019
View reviewed changes

@bnoordhuisbnoordhuis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turning string literals into mutable std::strings is kind of inelegant. It'd be better to have some kind of struct that lives in rodata:

extern"C"staticconststruct {
char arch[sizeof(NODE_ARCH)];
char platform[sizeof(NODE_PLATFORM)];
// etc.
} node_metadata = {
NODE_ARCH,
NODE_PLATFORM,
// etc.
};

Comment threadsrc/node_metadata.h Outdated
@joyeecheung

joyeecheung commented Jan 4, 2019•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

I've marked per_process:metadata.release, per_process:metadata.arch and per_process:metadata.platform added in this PR const. It should be possible to move the non-intl version strings const too, but I'd prefer leave that to another PR (also it's probably better if we make sure Metadata is a singleton and all its members are static)

CI: https://ci.nodejs.org/job/node-test-pull-request/19932/

addaleax
addaleax approved these changes Jan 4, 2019
View reviewed changes

@addaleaxaddaleax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just reaffirming my LGTM

@joyeecheungjoyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 4, 2019
@joyeecheung

joyeecheung commented Jan 6, 2019

Copy link
Copy Markdown
MemberAuthor

Landed in 6c01621

joyeecheung added a commit that referenced this pull request Jan 6, 2019
@joyeecheung
src: move arch, platform and release into node_metadata.cc
6c01621
Move definitions of more metadata into node_metadata{.h, .cc}
so the data can be reused and easily inspected in C++.
PR-URL: #25293
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax

addaleax commented Jan 9, 2019

Copy link
Copy Markdown
Member

This needs to be backported to v11.x.

refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@joyeecheung@refack
src: move arch, platform and release into node_metadata.cc
b028251
Move definitions of more metadata into node_metadata{.h, .cc}
so the data can be reused and easily inspected in C++.
PR-URL: nodejs#25293
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax

addaleax commented Jan 14, 2019

Copy link
Copy Markdown
Member

This applies cleanly now.

addaleax pushed a commit that referenced this pull request Jan 14, 2019
@joyeecheung@addaleax
src: move arch, platform and release into node_metadata.cc
e9b4d24
Move definitions of more metadata into node_metadata{.h, .cc}
so the data can be reused and easily inspected in C++.
PR-URL: #25293
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeARBridgeAR mentioned this pull request Jan 16, 2019
Merged
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
@joyeecheung@BridgeAR
src: move arch, platform and release into node_metadata.cc
3d4fe73
Move definitions of more metadata into node_metadata{.h, .cc}
so the data can be reused and easily inspected in C++.
PR-URL: nodejs#25293
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorinsMylesBorins mentioned this pull request Jan 24, 2019
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuisbnoordhuisbnoordhuis left review comments
@jasnelljasnelljasnell approved these changes
@addaleaxaddaleaxaddaleax approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.c++Issues and PRs that require attention from people who are familiar with C++.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@joyeecheung@nodejs-github-bot@addaleax@bnoordhuis@jasnell@BridgeAR