Skip to content

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Aug 21, 2025

The inspector command line flags should be allowed via NODE_OPTIONS, though it should not be consumed when passed directly to the SEA. This patch allows it to consume inspector flags from NODE_OPTIONS and add tests for different behaviors.

Combined with #59560, users can still disable inspector options in NODE_OPTIONS if they set execArgvExtension to either none or cli. They could alos enable it via the special CLI flag --node-options="--inspect" if they set it to cli in the configuration.

Fixes: #51688

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/config
  • @nodejs/single-executable

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. config Issues or PRs related to the config subsystem needs-ci PRs that need a full CI run. labels Aug 21, 2025
Copy link

codecov bot commented Aug 21, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.84%. Comparing base (6722642) to head (efb8376).
⚠️ Report is 16 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #59568      +/-   ##
==========================================
- Coverage   89.84%   89.84%   -0.01%     
==========================================
  Files         667      667              
  Lines      196260   196259       -1     
  Branches    38563    38566       +3     
==========================================
- Hits       176332   176327       -5     
+ Misses      12382    12377       -5     
- Partials     7546     7555       +9     
Files with missing lines Coverage Δ
src/node_options.cc 84.44% <ø> (-0.02%) ⬇️

... and 39 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

@joyeecheung
Copy link
Member Author

Removed verifyWorkflow = true in generateSEA() calls, since apparently one of the Windows machine in the CI does not have signtool available (not sure why I put it into these tests, probably just too much tabbing from autocomplete)

The inspector command line flags should be allowed via NODE_OPTIONS,
though it should not be consumed when passed directly to the SEA.
This patch allows it to consume inspector flags from NODE_OPTIONS
and add tests for different behaviors.
@joyeecheung
Copy link
Member Author

Rebased and added skip entries to sync up with #59563

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@joyeecheung
Copy link
Member Author

CI is green. This needs a re-approval to land. @jasnell @addaleax @RaisinTen can you take another look please? Thanks!

@joyeecheung joyeecheung added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Aug 27, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 27, 2025
@nodejs-github-bot nodejs-github-bot merged commit bcb802c into nodejs:main Aug 27, 2025
69 of 71 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in bcb802c

targos pushed a commit that referenced this pull request Aug 28, 2025
The inspector command line flags should be allowed via NODE_OPTIONS,
though it should not be consumed when passed directly to the SEA.
This patch allows it to consume inspector flags from NODE_OPTIONS
and add tests for different behaviors.

PR-URL: #59568
Fixes: #51688
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
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. config Issues or PRs related to the config subsystem needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow inspector flags with SEA binaries via configuration
6 participants