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

src: fix --disable-single-executable-application #51808

Closed
wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Feb 19, 2024

Previously it would not compile if the build is configured with
--disable-single-executable-application because we use directives
to exclude the definition of SEA-related code completely. This patch
changes them so that the SEA code are still compiled and internals
can still check whether the executable is an SEA. The executable would
not try to load the SEA blob at all if SEA is disabled. If future
modifications to the C++ code attempt to load the SEA blob when SEA
is disabled, UNREACHABLE() would be raised. If user attempt to
generate the SEA blob with --experimental-sea-config with an executable
that disables SEA, they would get an error.

Fixes: #51730

@nodejs-github-bot
Copy link
Collaborator

Review requested:

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

@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 Feb 19, 2024
Previously it would not compile if the build is configured with
--disable-single-executable-application because we use directives
to exclude the definition of SEA-related code completely. This patch
changes them so that the SEA code are still compiled and internals
can still check whether the executable is an SEA. The executable would
not try to load the SEA blob at all if SEA is disabled. If future
modifications to the C++ code attempt to load the SEA blob when SEA
is disabled, UNREACHABLE() would be raised. If user attempt to
generate the SEA blob with --experimental-sea-config with an executable
that disables SEA, they would get an error.
Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

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

RSLGTM

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 20, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 20, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 22, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 22, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 25, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 25, 2024
@nodejs-github-bot
Copy link
Collaborator

@joyeecheung joyeecheung added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 26, 2024
@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 Feb 26, 2024
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/51808
✔  Done loading data for nodejs/node/pull/51808
----------------------------------- PR info ------------------------------------
Title      src: fix --disable-single-executable-application (#51808)
Author     Joyee Cheung  (@joyeecheung)
Branch     joyeecheung:fix-disable-sea -> nodejs:main
Labels     c++, needs-ci, single-executable
Commits    1
 - src: fix --disable-single-executable-application
Committers 1
 - Joyee Cheung 
PR-URL: https://github.com/nodejs/node/pull/51808
Fixes: https://github.com/nodejs/node/issues/51730
Reviewed-By: Tobias Nießen 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/51808
Fixes: https://github.com/nodejs/node/issues/51730
Reviewed-By: Tobias Nießen 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Mon, 19 Feb 2024 18:28:53 GMT
   ✔  Approvals: 1
   ✔  - Tobias Nießen (@tniessen) (TSC): https://github.com/nodejs/node/pull/51808#pullrequestreview-1891732647
   ✘  This PR needs to wait 16 more hours to land (or 0 hours if there is one more approval)
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-02-25T23:57:03Z: https://ci.nodejs.org/job/node-test-pull-request/57415/
- Querying data for job/node-test-pull-request/57415/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/8042448915

@joyeecheung joyeecheung removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Feb 26, 2024
@joyeecheung joyeecheung added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 26, 2024
@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 Feb 26, 2024
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/51808
✔  Done loading data for nodejs/node/pull/51808
----------------------------------- PR info ------------------------------------
Title      src: fix --disable-single-executable-application (#51808)
Author     Joyee Cheung  (@joyeecheung)
Branch     joyeecheung:fix-disable-sea -> nodejs:main
Labels     c++, needs-ci, single-executable
Commits    1
 - src: fix --disable-single-executable-application
Committers 1
 - Joyee Cheung 
PR-URL: https://github.com/nodejs/node/pull/51808
Fixes: https://github.com/nodejs/node/issues/51730
Reviewed-By: Tobias Nießen 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/51808
Fixes: https://github.com/nodejs/node/issues/51730
Reviewed-By: Tobias Nießen 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Mon, 19 Feb 2024 18:28:53 GMT
   ✔  Approvals: 1
   ✔  - Tobias Nießen (@tniessen) (TSC): https://github.com/nodejs/node/pull/51808#pullrequestreview-1891732647
   ✘  This PR needs to wait 5 more hours to land (or 0 hours if there is one more approval)
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-02-26T01:41:38Z: https://ci.nodejs.org/job/node-test-pull-request/57415/
- Querying data for job/node-test-pull-request/57415/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/8049457418

@joyeecheung joyeecheung added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. commit-queue Add this label to land a pull request using GitHub Actions. labels Feb 27, 2024
@joyeecheung
Copy link
Member Author

Somehow I added a commit from #51887 - removed.

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 27, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 27, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

joyeecheung added a commit that referenced this pull request Mar 1, 2024
Previously it would not compile if the build is configured with
--disable-single-executable-application because we use directives
to exclude the definition of SEA-related code completely. This patch
changes them so that the SEA code are still compiled and internals
can still check whether the executable is an SEA. The executable would
not try to load the SEA blob at all if SEA is disabled. If future
modifications to the C++ code attempt to load the SEA blob when SEA
is disabled, UNREACHABLE() would be raised. If user attempt to
generate the SEA blob with --experimental-sea-config with an executable
that disables SEA, they would get an error.

PR-URL: #51808
Fixes: #51730
Reviewed-By: Tobias Nießen <[email protected]>
@joyeecheung
Copy link
Member Author

Landed in 01cb943

@joyeecheung joyeecheung closed this Mar 1, 2024
targos pushed a commit that referenced this pull request Mar 7, 2024
Previously it would not compile if the build is configured with
--disable-single-executable-application because we use directives
to exclude the definition of SEA-related code completely. This patch
changes them so that the SEA code are still compiled and internals
can still check whether the executable is an SEA. The executable would
not try to load the SEA blob at all if SEA is disabled. If future
modifications to the C++ code attempt to load the SEA blob when SEA
is disabled, UNREACHABLE() would be raised. If user attempt to
generate the SEA blob with --experimental-sea-config with an executable
that disables SEA, they would get an error.

PR-URL: #51808
Fixes: #51730
Reviewed-By: Tobias Nießen <[email protected]>
@targos targos mentioned this pull request Mar 7, 2024
richardlau pushed a commit that referenced this pull request Mar 25, 2024
Previously it would not compile if the build is configured with
--disable-single-executable-application because we use directives
to exclude the definition of SEA-related code completely. This patch
changes them so that the SEA code are still compiled and internals
can still check whether the executable is an SEA. The executable would
not try to load the SEA blob at all if SEA is disabled. If future
modifications to the C++ code attempt to load the SEA blob when SEA
is disabled, UNREACHABLE() would be raised. If user attempt to
generate the SEA blob with --experimental-sea-config with an executable
that disables SEA, they would get an error.

PR-URL: #51808
Fixes: #51730
Reviewed-By: Tobias Nießen <[email protected]>
richardlau pushed a commit that referenced this pull request Mar 25, 2024
Previously it would not compile if the build is configured with
--disable-single-executable-application because we use directives
to exclude the definition of SEA-related code completely. This patch
changes them so that the SEA code are still compiled and internals
can still check whether the executable is an SEA. The executable would
not try to load the SEA blob at all if SEA is disabled. If future
modifications to the C++ code attempt to load the SEA blob when SEA
is disabled, UNREACHABLE() would be raised. If user attempt to
generate the SEA blob with --experimental-sea-config with an executable
that disables SEA, they would get an error.

PR-URL: #51808
Fixes: #51730
Reviewed-By: Tobias Nießen <[email protected]>
@richardlau richardlau mentioned this pull request Mar 25, 2024
rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Mar 26, 2024
Previously it would not compile if the build is configured with
--disable-single-executable-application because we use directives
to exclude the definition of SEA-related code completely. This patch
changes them so that the SEA code are still compiled and internals
can still check whether the executable is an SEA. The executable would
not try to load the SEA blob at all if SEA is disabled. If future
modifications to the C++ code attempt to load the SEA blob when SEA
is disabled, UNREACHABLE() would be raised. If user attempt to
generate the SEA blob with --experimental-sea-config with an executable
that disables SEA, they would get an error.

PR-URL: nodejs#51808
Fixes: nodejs#51730
Reviewed-By: Tobias Nießen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

build: --disable-single-executable-application results in link failure
6 participants