GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — src: use Blob{Des|S}erializer for SEA blobs by joyeecheung · Pull Request #47962 · nodejs/node

Conversation

@joyeecheung

@joyeecheung joyeecheung commented May 11, 2023

Copy link
Copy Markdown
Member

No description provided.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@joyeecheung
src: use Blob{Des|S}erializer for SEA blobs
66c599a
@nodejs-github-bot

nodejs-github-bot commented May 11, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/single-executable
  • @nodejs/startup
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels May 11, 2023
anonrig
anonrig reviewed May 11, 2023
View reviewed changes
Comment thread src/blob_serializer_deserializer-inl.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread src/blob_serializer_deserializer-inl.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread src/blob_serializer_deserializer-inl.h
}

template <typename Impl>
size_t BlobSerializer<Impl>::WriteString(const std::string& data) {

@anonrig anonrig May 11, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Suggested change
size_t BlobSerializer<Impl>::WriteString(const std::string& data) {
size_t BlobSerializer<Impl>::WriteString(const std::string_view data) {

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@joyeecheung joyeecheung May 12, 2023

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This is the method that writes strings (which logs the string directly in debug mode). The one that doesn't is WriteStringView (which obviously doesn't log that data as string).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
kvakil
kvakil reviewed May 12, 2023
View reviewed changes
Comment thread src/node_sea.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread src/blob_serializer_deserializer-inl.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

@RaisinTen RaisinTen added the single-executable Issues and PRs related to single-executable applications. label May 12, 2023
RaisinTen
RaisinTen reviewed May 12, 2023
View reviewed changes

@RaisinTen RaisinTen left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Now that we're using these classes for both snapshots and SEAs, we can update these comments to also mention SEA:

node/src/blob_serializer_deserializer.h

Lines 9 to 10 in 2e67401

// This is related to the blob that is used in snapshots and has nothing to do
// with `node_blob.h`.

node/src/blob_serializer_deserializer-inl.h

Lines 16 to 17 in 2e67401

// This is related to the blob that is used in snapshots and has nothing to do
// with `node_blob.h`.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread src/node_sea.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread src/blob_serializer_deserializer-inl.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread src/blob_serializer_deserializer-inl.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread src/blob_serializer_deserializer-inl.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread src/blob_serializer_deserializer-inl.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread src/node_sea.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.

joyeecheung added 3 commits May 12, 2023 20:49
@joyeecheung
fixup! src: use Blob{Des|S}erializer for SEA blobs
7f90459
@joyeecheung
fixup! fixup! src: use Blob{Des|S}erializer for SEA blobs
1e4dfec
@joyeecheung
fixup! fixup! fixup! src: use Blob{Des|S}erializer for SEA blobs
a1ca8dc
RaisinTen
RaisinTen reviewed May 12, 2023
View reviewed changes
Comment thread src/node_sea.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread src/node_sea.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread test/sequential/test-single-executable-application-empty.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread src/blob_serializer_deserializer-inl.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread src/node_sea.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread src/node_sea.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.

@joyeecheung
fixup! fixup! fixup! fixup! src: use Blob{Des|S}erializer for SEA blobs
87c72f7
@nodejs-github-bot

nodejs-github-bot commented May 18, 2023

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

RaisinTen
RaisinTen approved these changes May 18, 2023
View reviewed changes

@RaisinTen RaisinTen left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM 👍 (just one typo)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread test/sequential/test-single-executable-application-empty.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

@joyeecheung @RaisinTen
fixup! src: use Blob{Des|S}erializer for SEA blobs
ef3fb40
Co-authored-by: Darshan Sen <[email protected]>
RaisinTen
RaisinTen approved these changes May 23, 2023
View reviewed changes
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
aymen94
aymen94 approved these changes May 23, 2023
View reviewed changes
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

nodejs-github-bot commented May 23, 2023

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@RaisinTen RaisinTen added author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue PRs queued for automated landing through the Commit Queue. commit-queue-squash PRs the Commit Queue should land as one squashed commit. labels May 23, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label May 23, 2023
@nodejs-github-bot
nodejs-github-bot merged commit e2caafa into nodejs:main May 23, 2023
@nodejs-github-bot

nodejs-github-bot commented May 23, 2023

Copy link
Copy Markdown
Collaborator

Landed in e2caafa

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@github-actions github-actions Bot mentioned this pull request May 24, 2023
CI Reliability 2023-05-24 nodejs/reliability#575
Open
19 tasks
@targos

targos commented May 24, 2023

Copy link
Copy Markdown
Member

I'm getting compiler errors and warnings with GCC 13:

  g++ -o /home/iojs/build/workspace/node-test-commit-linux/out/Release/obj.target/libnode/src/node_sea.o ../src/node_sea.cc '-DV8_DEPRECATION_WARNINGS' '-DV8_IMMINENT_DEPRECATION_WARNINGS' '-D_GLIBCXX_USE_CXX11_ABI=1' '-DNODE_OPENSSL_CONF_NAME=nodejs_conf' '-DNODE_OPENSSL_HAS_QUIC' '-DICU_NO_USER_DATA_OVERRIDE' '-D__STDC_FORMAT_MACROS' '-DOPENSSL_NO_PINSHARED' '-DOPENSSL_THREADS' '-DNODE_ARCH="x64"' '-DNODE_PLATFORM="linux"' '-DNODE_WANT_INTERNALS=1' '-DV8_DEPRECATION_WARNINGS=1' '-DNODE_OPENSSL_SYSTEM_CERT_PATH=""' '-DNODE_USE_NODE_CODE_CACHE=1' '-DHAVE_INSPECTOR=1' '-DNODE_ENABLE_LARGE_CODE_PAGES=1' '-D__POSIX__' '-DNODE_USE_V8_PLATFORM=1' '-DNODE_V8_SHARED_RO_HEAP' '-DNODE_HAVE_I18N_SUPPORT=1' '-DHAVE_OPENSSL=1' '-DOPENSSL_API_COMPAT=0x10100000L' '-DBASE64_STATIC_DEFINE' '-DUCONFIG_NO_SERVICE=1' '-DU_ENABLE_DYLOAD=0' '-DU_STATIC_IMPLEMENTATION=1' '-DU_HAVE_STD_STRING=1' '-DUCONFIG_NO_BREAK_ITERATION=0' '-D_LARGEFILE_SOURCE' '-D_FILE_OFFSET_BITS=64' '-D_POSIX_C_SOURCE=200112' '-DNGHTTP2_STATICLIB' '-DNDEBUG' '-DOPENSSL_USE_NODELETE' '-DL_ENDIAN' '-DOPENSSL_BUILDING_OPENSSL' '-DAES_ASM' '-DBSAES_ASM' '-DCMLL_ASM' '-DECP_NISTZ256_ASM' '-DGHASH_ASM' '-DKECCAK1600_ASM' '-DMD5_ASM' '-DOPENSSL_BN_ASM_GF2m' '-DOPENSSL_BN_ASM_MONT' '-DOPENSSL_BN_ASM_MONT5' '-DOPENSSL_CPUID_OBJ' '-DOPENSSL_IA32_SSE2' '-DPADLOCK_ASM' '-DPOLY1305_ASM' '-DSHA1_ASM' '-DSHA256_ASM' '-DSHA512_ASM' '-DVPAES_ASM' '-DWHIRLPOOL_ASM' '-DX25519_ASM' '-DOPENSSL_PIC' '-DNGTCP2_STATICLIB' '-DNGHTTP3_STATICLIB' -I../src -I../deps/postject -I/home/iojs/build/workspace/node-test-commit-linux/out/Release/obj/gen -I/home/iojs/build/workspace/node-test-commit-linux/out/Release/obj/gen/include -I/home/iojs/build/workspace/node-test-commit-linux/out/Release/obj/gen/src -I../deps/base64/base64/include -I../deps/googletest/include -I../deps/histogram/src -I../deps/uvwasi/include -I../deps/simdutf -I../deps/ada -I../deps/v8/include -I../deps/icu-small/source/i18n -I../deps/icu-small/source/common -I../deps/zlib -I../deps/llhttp/include -I../deps/cares/include -I../deps/uv/include -I../deps/nghttp2/lib/includes -I../deps/brotli/c/include -I../deps/openssl/openssl/include -I../deps/openssl/openssl/crypto/include -I../deps/openssl/config/archs/linux-x86_64/asm/include -I../deps/openssl/config/archs/linux-x86_64/asm -I../deps/ngtcp2 -I../deps/ngtcp2/ngtcp2/lib/includes -I../deps/ngtcp2/ngtcp2/crypto/includes -I../deps/ngtcp2/ngtcp2/crypto -I../deps/ngtcp2/nghttp3/lib/includes  -Wall -Wextra -Wno-unused-parameter -pthread -Wall -Wextra -Wno-unused-parameter -m64 -Werror=unused-result -O3 -fno-omit-frame-pointer -fno-rtti -fno-exceptions -std=gnu++17 -MMD -MF /home/iojs/build/workspace/node-test-commit-linux/out/Release/.deps//home/iojs/build/workspace/node-test-commit-linux/out/Release/obj.target/libnode/src/node_sea.o.d.raw   -c
In file included from ../src/node_sea.cc:1:
../src/node_sea.h:18:7: error: ‘uint32_t’ does not name a type
   18 | const uint32_t kMagic = 0x143da20;
      |       ^~~~~~~~
../src/node_sea.h:11:1: note: ‘uint32_t’ is defined in header ‘<cstdint>’; did you forget to ‘#include <cstdint>’?
   10 | #include "node_exit_code.h"
  +++ |+#include <cstdint>
   11 |
../src/node_sea.h:20:6: warning: elaborated-type-specifier for a scoped enum must not use the ‘class’ keyword
   20 | enum class SeaFlags : uint32_t {
      | ~~~~ ^~~~~
      |      -----
../src/node_sea.h:20:21: error: found ‘:’ in nested-name-specifier, expected ‘::’
   20 | enum class SeaFlags : uint32_t {
      |                     ^
      |                     ::
../src/node_sea.h:20:12: error: ‘SeaFlags’ has not been declared
   20 | enum class SeaFlags : uint32_t {
      |            ^~~~~~~~
../src/node_sea.h:20:32: error: expected unqualified-id before ‘{’ token
   20 | enum class SeaFlags : uint32_t {
      |                                ^
../src/node_sea.h:26:3: error: ‘SeaFlags’ does not name a type
   26 |   SeaFlags flags = SeaFlags::kDefault;
      |   ^~~~~~~~
../src/node_sea.h:29:48: error: ‘kMagic’ was not declared in this scope
   29 |   static constexpr size_t kHeaderSize = sizeof(kMagic) + sizeof(SeaFlags);
      |                                                ^~~~~~
../src/node_sea.h:29:65: error: ‘SeaFlags’ was not declared in this scope
   29 |   static constexpr size_t kHeaderSize = sizeof(kMagic) + sizeof(SeaFlags);
      |                                                                 ^~~~~~~~
../src/node_sea.cc:39:1: error: ‘SeaFlags’ does not name a type
   39 | SeaFlags operator|(SeaFlags x, SeaFlags y) {
      | ^~~~~~~~
../src/node_sea.cc:44:1: error: ‘SeaFlags’ does not name a type
   44 | SeaFlags operator&(SeaFlags x, SeaFlags y) {
      | ^~~~~~~~
../src/node_sea.cc:49:1: error: ‘SeaFlags’ does not name a type
   49 | SeaFlags operator|=(/* NOLINT (runtime/references) */ SeaFlags& x, SeaFlags y) {
      | ^~~~~~~~
../src/node_sea.cc: In member function ‘size_t node::sea::{anonymous}::SeaSerializer::Write(const T&) [with T = node::sea::SeaResource; std::enable_if_t<(! std::is_same<T, std::__cxx11::basic_string<char> >::value)>* <anonymous> = 0; std::enable_if_t<(! std::is_arithmetic<_Tp>::value)>* <anonymous> = 0; size_t = long unsigned int]’:
../src/node_sea.cc:69:33: error: ‘kMagic’ was not declared in this scope
   69 |   Debug("Write SEA magic %x\n", kMagic);
      |                                 ^~~~~~
../src/node_sea.cc:72:46: error: ‘const struct node::sea::SeaResource’ has no member named ‘flags’
   72 |   uint32_t flags = static_cast<uint32_t>(sea.flags);
      |                                              ^~~~~
In file included from ../src/debug_utils.h:7,
                 from ../src/debug_utils-inl.h:6,
                 from ../src/blob_serializer_deserializer-inl.h:14,
                 from ../src/node_sea.cc:3:
../src/node_sea.cc: In member function ‘T node::sea::{anonymous}::SeaDeserializer::Read() [with T = node::sea::SeaResource; std::enable_if_t<(! std::is_same<T, std::__cxx11::basic_string<char> >::value)>* <anonymous> = 0; std::enable_if_t<(! std::is_arithmetic<_Tp>::value)>* <anonymous> = 0]’:
../src/node_sea.cc:101:19: error: ‘kMagic’ was not declared in this scope; did you mean ‘magic’?
  101 |   CHECK_EQ(magic, kMagic);
      |                   ^~~~~~
../src/util.h:141:44: note: in definition of macro ‘UNLIKELY’
  141 | #define UNLIKELY(expr) __builtin_expect(!!(expr), 0)
      |                                            ^~~~
../src/util.h:159:24: note: in expansion of macro ‘CHECK’
  159 | #define CHECK_EQ(a, b) CHECK((a) == (b))
      |                        ^~~~~
../src/node_sea.cc:101:3: note: in expansion of macro ‘CHECK_EQ’
  101 |   CHECK_EQ(magic, kMagic);
      |   ^~~~~~~~
../src/node_sea.cc:102:3: error: ‘SeaFlags’ was not declared in this scope; did you mean ‘StopFlags’?
  102 |   SeaFlags flags(static_cast<SeaFlags>(ReadArithmetic<uint32_t>()));
      |   ^~~~~~~~
      |   StopFlags
../src/node_sea.cc:103:54: error: ‘flags’ was not declared in this scope
  103 |   Debug("Read SEA flags %x\n", static_cast<uint32_t>(flags));
      |                                                      ^~~~~
../src/node_sea.cc:108:22: error: could not convert ‘{flags, code}’ from ‘<brace-enclosed initializer list>’ to ‘node::sea::SeaResource’
  108 |   return {flags, code};
      |                      ^
      |                      |
      |                      <brace-enclosed initializer list>
../src/node_sea.cc: In function ‘void node::sea::IsExperimentalSeaWarningNeeded(const v8::FunctionCallbackInfo<v8::Value>&)’:
../src/node_sea.cc:161:20: error: ‘struct node::sea::SeaResource’ has no member named ‘flags’
  161 |       sea_resource.flags & SeaFlags::kDisableExperimentalSeaWarning));
      |                    ^~~~~
../src/node_sea.cc:161:28: error: ‘SeaFlags’ has not been declared
  161 |       sea_resource.flags & SeaFlags::kDisableExperimentalSeaWarning));
      |                            ^~~~~~~~
../src/node_sea.cc: At global scope:
../src/node_sea.cc:185:3: error: ‘SeaFlags’ does not name a type
  185 |   SeaFlags flags = SeaFlags::kDefault;
      |   ^~~~~~~~
../src/node_sea.cc: In function ‘std::optional<node::sea::{anonymous}::SeaConfig> node::sea::{anonymous}::ParseSingleExecutableConfig(const std::string&)’:
../src/node_sea.cc:235:12: error: ‘struct node::sea::{anonymous}::SeaConfig’ has no member named ‘flags’
  235 |     result.flags |= SeaFlags::kDisableExperimentalSeaWarning;
      |            ^~~~~
../src/node_sea.cc:235:21: error: ‘SeaFlags’ has not been declared
  235 |     result.flags |= SeaFlags::kDisableExperimentalSeaWarning;
      |                     ^~~~~~~~
../src/node_sea.cc: In function ‘node::ExitCode node::sea::{anonymous}::GenerateSingleExecutableBlob(const SeaConfig&)’:
../src/node_sea.cc:251:26: error: ‘const struct node::sea::{anonymous}::SeaConfig’ has no member named ‘flags’
  251 |   SeaResource sea{config.flags, main_script};
      |                          ^~~~~
make[2]: *** [libnode.target.mk:490: /home/iojs/build/workspace/node-test-commit-linux/out/Release/obj.target/libnode/src/node_sea.o] Error 1
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@joyeecheung joyeecheung mentioned this pull request May 24, 2023
src: include missing header in node_sea.h #48152
Merged
@joyeecheung

joyeecheung commented May 24, 2023

Copy link
Copy Markdown
Member Author

@targos does this fix the error? #48152

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@targos

targos commented May 24, 2023

Copy link
Copy Markdown
Member

@joyeecheung yes it does

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This was referenced May 25, 2023
CI Reliability 2023-05-25 nodejs/reliability#576
Open
CI Reliability 2023-05-26 nodejs/reliability#577
Open
CI Reliability 2023-05-27 nodejs/reliability#578
Open
CI Reliability 2023-05-28 nodejs/reliability#579
Open
CI Reliability 2023-05-29 nodejs/reliability#580
Open
CI Reliability 2023-05-30 nodejs/reliability#581
Open
targos pushed a commit that referenced this pull request May 30, 2023
@joyeecheung @targos
src: use Blob{Des|S}erializer for SEA blobs
33aa373
PR-URL: #47962
Reviewed-By: Darshan Sen <[email protected]>
@github-actions github-actions Bot mentioned this pull request May 31, 2023
CI Reliability 2023-05-31 nodejs/reliability#582
Open
22 tasks
@targos targos mentioned this pull request Jun 4, 2023
v20.3.0 release proposal #48332
Merged
@danielleadams danielleadams added the backport-requested-v18.x label Jun 30, 2023
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@joyeecheung @Ceres6
src: use Blob{Des|S}erializer for SEA blobs
d62da64
PR-URL: nodejs#47962
Reviewed-By: Darshan Sen <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@joyeecheung @Ceres6
src: use Blob{Des|S}erializer for SEA blobs
a184582
PR-URL: nodejs#47962
Reviewed-By: Darshan Sen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anonrig anonrig anonrig left review comments
@RaisinTen RaisinTen RaisinTen approved these changes
+2 more reviewers
@kvakil kvakil kvakil left review comments
@aymen94 aymen94 aymen94 approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash PRs the Commit Queue should land as one squashed commit. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. single-executable Issues and PRs related to single-executable applications.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@joyeecheung @nodejs-github-bot @targos @anonrig @aymen94 @kvakil @RaisinTen @danielleadams
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.