Skip to content

src: add support for externally shared js builtins #44376

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

Closed
mhdawson wants to merge 6 commits into from
Closed

src: add support for externally shared js builtins #44376

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b7d4bcd
src: add support for externally shared js builtins
mhdawson Aug 24, 2022
5428aa2
squash: address linters
mhdawson Aug 26, 2022
5dd4c99
Update doc/contributing/maintaining-dependencies.md
mhdawson Sep 2, 2022
6f6d80e
squash: address comments
mhdawson Sep 2, 2022
a614c7b
squash: fixup
mhdawson Sep 9, 2022
eb708c8
Update doc/contributing/maintaining-dependencies.md
mhdawson Sep 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
squash: address comments 
Signed-off-by: Michael Dawson <mdawson@devrus.com>
  • Loading branch information
@mhdawson
mhdawson committed Sep 2, 2022
commit 6f6d80eb7c9923a2427dde09093e029a23cf02a4
12 changes: 6 additions & 6 deletions configure.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@
with open ('tools/icu/icu_versions.json') as f:
icu_versions = json.load(f)

sharable_builtins = {'cjs_module_lexer/lexer': 'deps/cjs-module-lexer/lexer.js',
shareable_builtins = {'cjs_module_lexer/lexer': 'deps/cjs-module-lexer/lexer.js',
'cjs_module_lexer/dist/lexer': 'deps/cjs-module-lexer/dist/lexer.js',
'undici/undici': 'deps/undici/undici.js'
Copy link
Copy Markdown
Member

@addaleax addaleax Aug 25, 2022

Choose a reason for hiding this comment

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

I assume this means that distros can now provide different versions of these dependencies. Are we okay with that?

Copy link
Copy Markdown
Member Author

@mhdawson mhdawson Aug 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I see this as very similar to supporting external shared libraries and it is motivated by a similar use case. Since it does not affect what we ship I think it's up to the distros that use it to keep the versions the same or compatible in what they ship just like they do for shared libraries.

Copy link
Copy Markdown
Member

@jasnell jasnell Aug 28, 2022

Choose a reason for hiding this comment

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

I think we need to carefully weigh this. I see the motivation but not quite convinced yet.

Copy link
Copy Markdown
Member Author

@mhdawson mhdawson Sep 2, 2022

Choose a reason for hiding this comment

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

@jasnell can you outline your concerns in a bit more detail? We have 2 distro's asking for the functionality in order to avoid having to hack in their own solutions?

}
Expand Down Expand Up @@ -430,7 +430,7 @@

parser.add_argument_group(shared_optgroup)

for builtin in sharable_builtins:
for builtin in shareable_builtins:
builtin_id = 'shared_builtin_' + builtin + '_path'
shared_builtin_optgroup.add_argument('--shared-builtin-' + builtin + '-path',
action='store',
Expand Down Expand Up @@ -1969,9 +1969,9 @@ def make_bin_override():
configure_inspector(output)
configure_section_file(output)

# configure sharable builtins
output['variables']['node_builtin_sharable_builtins'] = []
for builtin in sharable_builtins:
# configure shareable builtins
output['variables']['node_builtin_shareable_builtins'] = []
for builtin in shareable_builtins:
builtin_id = 'node_shared_builtin_' + builtin.replace('/', '_') + '_path'
if getattr(options, builtin_id):
if options.with_intl == 'none':
Expand All @@ -1980,7 +1980,7 @@ def make_bin_override():
else:
output['defines'] += [builtin_id.upper() + '=' + getattr(options, builtin_id)]
else:
output['variables']['node_builtin_sharable_builtins'] += [sharable_builtins[builtin]]
output['variables']['node_builtin_shareable_builtins'] += [shareable_builtins[builtin]]

# Forward OSS-Fuzz settings
output['variables']['ossfuzz'] = b(options.ossfuzz)
Expand Down
2 changes: 1 addition & 1 deletion node.gyp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@
'deps/v8/tools/tickprocessor-driver.mjs',
'deps/acorn/acorn/dist/acorn.js',
'deps/acorn/acorn-walk/dist/walk.js',
'<@(node_builtin_sharable_builtins)',
'<@(node_builtin_shareable_builtins)',
],
'node_mksnapshot_exec': '<(PRODUCT_DIR)/<(EXECUTABLE_PREFIX)node_mksnapshot<(EXECUTABLE_SUFFIX)',
'conditions': [
Expand Down