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.

Sign up for GitHub

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

Jump to bottom

sea: add option to disable the experimental SEA warning #47588

Merged
merged 26 commits into from
May 4, 2023
Merged

sea: add option to disable the experimental SEA warning #47588

merged 26 commits into from
May 4, 2023

Conversation

RaisinTen
Copy link
Contributor

@RaisinTen RaisinTen commented Apr 17, 2023 •
edited
Loading

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/single-executable

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. single-executable Issues and PRs related to single-executable applications labels Apr 17, 2023
@RaisinTen RaisinTen force-pushed the sea-add-option-to-disable-experimental-sea-warning branch 2 times, most recently from 55d4642 to c10ae9b Compare April 17, 2023 08:58
addaleax
addaleax reviewed Apr 17, 2023
View reviewed changes
src/json_parser.cc Outdated Show resolved Hide resolved
mhdawson
mhdawson reviewed Apr 18, 2023
View reviewed changes
src/json_parser.h Outdated Show resolved Hide resolved
mhdawson
mhdawson approved these changes Apr 18, 2023
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM. I agree that for some of the additional use cases discussed disabling only the SEA warning versus all warnings makes sense.

@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@RaisinTen RaisinTen added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Apr 18, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@RaisinTen RaisinTen added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 20, 2023
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Apr 20, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@github-actions github-actions bot mentioned this pull request Apr 24, 2023
Open
39 tasks
@RaisinTen RaisinTen removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Apr 24, 2023
joyeecheung
joyeecheung reviewed Apr 24, 2023
View reviewed changes
src/node_sea.cc Outdated Show resolved Hide resolved
bnb
bnb approved these changes Apr 24, 2023
View reviewed changes
Copy link
Contributor

@bnb bnb left a comment

Choose a reason for hiding this comment

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

RSLGTM for the idea

RaisinTen added a commit to RaisinTen/node that referenced this pull request Apr 25, 2023
@RaisinTen
sea: use a uint32_t bit field instead of bool disable_experimental_se…
d597344 
…a_warning

Refs: nodejs#47588 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>
@RaisinTen RaisinTen removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 25, 2023
@ovflowd
Copy link
Member

ovflowd commented Apr 25, 2023

ALSO SGTM for the idea. It honestly sounds fair.

joyeecheung
joyeecheung reviewed Apr 25, 2023
View reviewed changes
doc/api/single-executable-applications.md Outdated Show resolved Hide resolved
doc/api/single-executable-applications.md Outdated Show resolved Hide resolved
lib/internal/main/embedding.js Outdated Show resolved Hide resolved
src/node_sea.cc Outdated Show resolved Hide resolved
src/node_sea.cc Outdated Show resolved Hide resolved
RaisinTen added a commit to RaisinTen/node that referenced this pull request Apr 26, 2023
@RaisinTen
sea: use the SeaFlags enum class instead of std::bitset
22cb244 
Refs: nodejs#47588 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>
RaisinTen added a commit to RaisinTen/node that referenced this pull request Apr 26, 2023
@RaisinTen
sea: move flags before code in SeaResource struct
ce5e999 
Refs: nodejs#47588 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>
RaisinTen added a commit to RaisinTen/node that referenced this pull request Apr 26, 2023
@RaisinTen
sea: expose only isExperimentalSeaWarningNeeded to JS from the binding
fa9df64 
Refs: nodejs#47588 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>
RaisinTen added 8 commits May 3, 2023 20:00
@RaisinTen
src: support UTF8 fields in JSONParser
305b7a6 
Node.js uses UTF-8 for almost all things as the default internally, and
this method should not be an exception

Refs: nodejs#47588 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>
@RaisinTen
src: do proper Maybe usage
c2da60b 
It makes more sense to use a Maybe here because that conveys the meaning
that it is unsafe to call into V8 if an exception is pending. Using
std::optional does not make that obvious.

Refs: nodejs#47588 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>
@RaisinTen
test: move single executable E2E tests to sequential
eb858c1 
These became flaky on osx11-x64 Jenkins CI the moment we started running
multiple single-executable tests in parallel, so it makes sense to run
these sequentially for now.

Signed-off-by: Darshan Sen <raisinten@gmail.com>
@RaisinTen
test: convert the SEA skips into a helper in test/common
b3f437a 
Refs: nodejs#47588 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>
@RaisinTen
fixup! test: convert the SEA skips into a helper in test/common
83f996b 
Signed-off-by: Darshan Sen <raisinten@gmail.com>
@RaisinTen
Revert "src: do proper Maybe usage"
5d21e60 
This reverts commit 4bd6caf.

Refs: nodejs#47588 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>
@RaisinTen
src: define kHeaderSize and use it everywhere
009fe2f 
Refs: nodejs#47588 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>
@RaisinTen
test: wrap inject + code signing part into a helper
d6642b7 
Refs: nodejs#47588 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>
@RaisinTen RaisinTen force-pushed the sea-add-option-to-disable-experimental-sea-warning branch from 4d19a45 to d6642b7 Compare May 3, 2023 15:12
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
RaisinTen added 2 commits May 3, 2023 20:48
@RaisinTen
fixup! test: wrap inject + code signing part into a helper
3373bd5 
Signed-off-by: Darshan Sen <raisinten@gmail.com>
@RaisinTen
fixup! src: define kHeaderSize and use it everywhere
2b5804e 
Signed-off-by: Darshan Sen <raisinten@gmail.com>
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@github-actions github-actions bot mentioned this pull request May 4, 2023
Open
28 tasks
joyeecheung
joyeecheung approved these changes May 4, 2023
View reviewed changes
Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@nodejs-github-bot
Copy link
Collaborator

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

@RaisinTen RaisinTen added the commit-queue Add this label to land a pull request using GitHub Actions. label May 4, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 4, 2023
@nodejs-github-bot nodejs-github-bot merged commit 2de10f5 into nodejs:main May 4, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 2de10f5

@RaisinTen RaisinTen deleted the sea-add-option-to-disable-experimental-sea-warning branch May 4, 2023 15:28
@github-actions github-actions bot mentioned this pull request May 5, 2023
Open
22 tasks
targos pushed a commit that referenced this pull request May 12, 2023
@RaisinTen @targos
sea: add option to disable the experimental SEA warning
c4596b9 
Refs: nodejs/single-executable#60
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: #47588
Fixes: #47741
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Tierney Cyren <hello@bnb.im>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@targos targos added the semver-minor PRs that contain new features and should be released in the next minor version. label May 15, 2023
targos added a commit that referenced this pull request May 15, 2023
@targos
2023-05-16, Version 20.2.0 (Current)
258e9e7 
Notable changes:

doc:
  * add ovflowd to collaborators (Claudio Wunder) #47844
http:
  * (SEMVER-MINOR) prevent writing to the body when not allowed by HTTP spec (Gerrard Lindsay) #47732
sea:
  * (SEMVER-MINOR) add option to disable the experimental SEA warning (Darshan Sen) #47588
test_runner:
  * (SEMVER-MINOR) add `skip`, `todo`, and `only` shorthands to `test` (Chemi Atlow) #47909
url:
  * (SEMVER-MINOR) add value argument to `URLSearchParams` `has` and `delete` methods (Sankalp Shubham) #47885

PR-URL: #48020
@targos targos mentioned this pull request May 15, 2023
Merged
targos added a commit that referenced this pull request May 16, 2023
@targos
2023-05-16, Version 20.2.0 (Current)
c2ca429 
Notable changes:

doc:
  * add ovflowd to collaborators (Claudio Wunder) #47844
http:
  * (SEMVER-MINOR) prevent writing to the body when not allowed by HTTP spec (Gerrard Lindsay) #47732
sea:
  * (SEMVER-MINOR) add option to disable the experimental SEA warning (Darshan Sen) #47588
test_runner:
  * (SEMVER-MINOR) add `skip`, `todo`, and `only` shorthands to `test` (Chemi Atlow) #47909
url:
  * (SEMVER-MINOR) add value argument to `URLSearchParams` `has` and `delete` methods (Sankalp Shubham) #47885

PR-URL: #48020
@danielleadams danielleadams added the backport-blocked-v18.x PRs that should land on the v18.x-staging branch but are blocked by another PR's pending backport. label Jul 3, 2023
@danielleadams
Copy link
Contributor

Blocked by #47125

@RaisinTen RaisinTen mentioned this pull request Nov 5, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tony-go tony-go tony-go left review comments

@addaleax addaleax addaleax approved these changes

@aymen94 aymen94 aymen94 approved these changes

@mhdawson mhdawson mhdawson approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@bnb bnb bnb approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-blocked-v18.x PRs that should land on the v18.x-staging branch but are blocked by another PR's pending backport. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version. 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.

Flaky test-single-executable-application