-
-
Notifications
You must be signed in to change notification settings - Fork 35.5k
src: add NODE_OPTIONS_STANDALONE #59693
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
Open
devsnek
wants to merge
1
commit into
nodejs:main
Choose a base branch
from
devsnek:node-options-standalone
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+181
−59
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,108 @@ | ||
| #include "src/node_options.h" | ||
| #include "src/util.h" | ||
|
|
||
| #include <iostream> | ||
| #include <ranges> | ||
| #include <string> | ||
| #include <vector> | ||
|
|
||
| extern "C" { | ||
|
|
||
| uint64_t uv_get_total_memory() { | ||
| return 0; | ||
| } | ||
|
|
||
| uint64_t uv_get_constrained_memory() { | ||
| return 0; | ||
| } | ||
| }; | ||
|
|
||
| namespace node { | ||
|
|
||
| void Assert(const AssertionInfo& info) { | ||
| fprintf(stderr, | ||
| "\n" | ||
| " # %s at %s\n" | ||
| " # Assertion failed: %s\n\n", | ||
| info.function ? info.function : "(unknown function)", | ||
| info.file_line ? info.file_line : "(unknown source location)", | ||
| info.message); | ||
|
|
||
| fflush(stderr); | ||
|
|
||
| abort(); | ||
| } | ||
|
|
||
| } // namespace node | ||
|
|
||
| int main(int argc, char** argv) { | ||
| std::vector<std::string> args(argv, argv + argc); | ||
| std::vector<std::string> exec_args; | ||
| std::vector<std::string> errors; | ||
|
|
||
| node::PerProcessOptions cli_options; | ||
|
|
||
| cli_options.cmdline = args; | ||
|
|
||
| if (const char* result = std::getenv("NODE_OPTIONS")) { | ||
| std::string node_options(result); | ||
|
|
||
| std::vector<std::string> env_argv = | ||
| node::ParseNodeOptionsEnvVar(node_options, &errors); | ||
| for (auto error : errors) { | ||
| std::cout << "error: " << error << std::endl; | ||
| } | ||
| if (!errors.empty()) return 1; | ||
|
|
||
| env_argv.insert(env_argv.begin(), args.at(0)); | ||
|
|
||
| { | ||
| std::vector<std::string> v8_args; | ||
| node::options_parser::Parse(&env_argv, | ||
| nullptr, | ||
| &v8_args, | ||
| &cli_options, | ||
| node::OptionEnvvarSettings::kAllowedInEnvvar, | ||
| &errors); | ||
|
|
||
| for (auto arg : v8_args | std::views::drop(1)) { | ||
| std::cout << "v8_arg: " << arg << std::endl; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| node::HandleEnvOptions(cli_options.per_isolate->per_env, | ||
| [](const char* name) { | ||
| const char* value = std::getenv(name); | ||
| if (value != nullptr) { | ||
| return std::string(value); | ||
| } | ||
| return std::string(""); | ||
| }); | ||
|
|
||
| { | ||
| std::vector<std::string> v8_args; | ||
| node::options_parser::Parse(&args, | ||
| &exec_args, | ||
| &v8_args, | ||
| &cli_options, | ||
| node::OptionEnvvarSettings::kDisallowedInEnvvar, | ||
| &errors); | ||
|
|
||
| for (auto arg : v8_args | std::views::drop(1)) { | ||
| std::cout << "v8_arg: " << arg << std::endl; | ||
| } | ||
| } | ||
|
|
||
| for (auto arg : args) { | ||
| std::cout << "arg: " << arg << std::endl; | ||
| } | ||
|
|
||
| for (auto arg : exec_args) { | ||
| std::cout << "exec_arg: " << arg << std::endl; | ||
| } | ||
|
|
||
| for (auto error : errors) { | ||
| std::cout << "error: " << error << std::endl; | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.
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.
How would this be maintained going forward? For example, this creates a dependence on the current shape of
PerProcessOptionsetc, which are very much just ad-hoc internal data structures that are very subject to change. Things likeHandleEnvOptionsorParseare also subject to internal refactoring, like for example adding a different mechanism thanHandleEnvOptionsfor creating env var/flag pairs likeEnvImplies("NODE_PENDING_DEPRECATION", "--pending-deprecation")and then just create inline versions for different parsing code to reduce the use of lambdas then this would be ditched. We can also change the option categories like creating RealmOptions etc, or stop usingstd::strings but usestd::string_viewon a backing store.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.
i would not expect node to maintain any compatibility for this interface aside from somehow being able to compile it as a standalone file or set of files that doesn't need to link against uv/libnode/v8/etc.
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.
also if node decided at some point in the future that it was too annoying to maintain even that and removed the ifdefs or whatever it was, that would be understandable.
Uh oh!
There was an error while loading. Please reload this page.
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.
If compatibility is not a concern, then I think it makes more sense to just do a CI elsewhere instead of checking the test into the Node.js source, because then the maintainability burden would be shifted to Node.js contributors, and may create barriers to new contributors if they don't know the context and somehow think that breaking or removing this test is bad (think how many tests that are created > 10 years ago tend to give us a hard time in the CI because nobody knows what they are doing anymore or whether they can be removed because they are just irrelevant now). We can keep the macros internally to reduce the merge conflicts, but keeping the test in the CI doesn't seem very helpful if it's supposed to be maintained by another project. (I think every time I try to touch the node.h I have a lot of doubts about whether I am breaking something important, even with some GitHub archeology I still am doubtful and as a result, I think a lot of public APIs there are just rotting without users because nobody is certain whether they can be touched)
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.
I'm think it is not guaranteed that node_options.h/cc has no dependencies like uv/libnode/v8/etc.
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.
Yeah I feel that to address #46339 we might end up adding a v8 dependency to the option parser....