Skip to content

WIP: better linking codegen #2157

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
Open
wants to merge 6 commits into
base: zbarsky/internal
Choose a base branch
from
Open

WIP: better linking codegen #2157

wants to merge 6 commits into from

Conversation

dzbarsky
Copy link
Collaborator

@dzbarsky dzbarsky commented Apr 6, 2025

Changes are visible to end-users: yes/no

  • Searched for relevant documentation and updated as needed: yes/no
  • Breaking change (forces users to change their own code or config): yes/no
  • Suggested release notes appear below: yes/no

Test plan

  • Covered by existing test cases
  • New test cases added
  • Manual testing; please provide instructions so we can reproduce:

@aspect-workflows Aspect Workflows
Copy link

aspect-workflows bot commented Apr 6, 2025 •
edited
Loading

Test

All tests were cache hits

225 tests (100.0%) were fully cached saving 32s.

Test

e2e/bzlmod

All tests were cache hits

5 tests (100.0%) were fully cached saving 502ms.

Test

e2e/gyp_no_install_script

1 test target passed

Targets 
//:write_npm_translate_lock_bzlmod_test [k8-fastbuild]24ms

Total test execution time was 24ms. 1 test (50.0%) was fully cached saving 139ms.

Test

e2e/js_image_oci

All tests were cache hits

1 test (100.0%) was fully cached saving 2s.

Test

e2e/npm_link_package

All tests were cache hits

3 tests (100.0%) were fully cached saving 448ms.

Test

e2e/npm_link_package-esm

All tests were cache hits

3 tests (100.0%) were fully cached saving 533ms.

Test

e2e/npm_translate_lock

All tests were cache hits

1 test (100.0%) was fully cached saving 30ms.

Test

e2e/npm_translate_lock_empty

All tests were cache hits

1 test (100.0%) was fully cached saving 30ms.

Test

e2e/npm_translate_lock_multi

All tests were cache hits

2 tests (100.0%) were fully cached saving 268ms.

Test

e2e/npm_translate_lock_partial_clone

All tests were cache hits

1 test (100.0%) was fully cached saving 31ms.

Test

e2e/npm_translate_lock_replace_packages

All tests were cache hits

3 tests (100.0%) were fully cached saving 603ms.

Test

e2e/npm_translate_lock_subdir_patch

All tests were cache hits

1 test (100.0%) was fully cached saving 98ms.

Test

e2e/npm_translate_package_lock

All tests were cache hits

1 test (100.0%) was fully cached saving 31ms.

Test

e2e/npm_translate_yarn_lock

All tests were cache hits

1 test (100.0%) was fully cached saving 31ms.

Test

e2e/package_json_module

All tests were cache hits

1 test (100.0%) was fully cached saving 253ms.

Test

e2e/pnpm_lockfiles

All tests were cache hits

42 tests (100.0%) were fully cached saving 3s.

Test

e2e/pnpm_workspace

All tests were cache hits

10 tests (100.0%) were fully cached saving 2s.

Test

e2e/pnpm_workspace_rerooted

All tests were cache hits

12 tests (100.0%) were fully cached saving 2s.

Test

e2e/repo_mapping

All tests were cache hits

3 tests (100.0%) were fully cached saving 404ms.

Test

e2e/rules_foo

All tests were cache hits

2 tests (100.0%) were fully cached saving 187ms.

Test

e2e/runfiles

All tests were cache hits

1 test (100.0%) was fully cached saving 173ms.

Test

e2e/vendored_node

All tests were cache hits

1 test (100.0%) was fully cached saving 99ms.

Buildifier      Format

@dzbarsky dzbarsky force-pushed the zbarsky/links branch 2 times, most recently from 65b316c to 7bf715d Compare April 6, 2025 21:54
@dzbarsky dzbarsky requested a review from jbedard April 6, 2025 21:54
@dzbarsky dzbarsky force-pushed the zbarsky/links branch 2 times, most recently from 33445ac to 0f34833 Compare April 6, 2025 21:56
jbedard
jbedard reviewed Apr 7, 2025
View reviewed changes
e2e/gyp_no_install_script/snapshots/bzlmod/segfault-handler_defs.bzl
link_packages = {
"": ["segfault-handler"],
},
link_visibility = ["//visibility:public"],
Copy link
Member

Choose a reason for hiding this comment

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

I wonder about things like this as well. Are we constructing both an array and string here 100% of the time?

Copy link
Member

Choose a reason for hiding this comment

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

Can't some things like this be put into _npm_link_imported_package_store defaults and then just drop this line? Same with other params maybe?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah i was considering that, it would require a bit more changes to how we instantiate the templates but I agree it would be nice to be able to use better defaults. want me to do it here or a followup?

Copy link
Member

Choose a reason for hiding this comment

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

Probably a followup?

jbedard
jbedard reviewed Apr 7, 2025
View reviewed changes
npm/private/npm_import.bzl

deps = {deps}
ref_deps = {ref_deps}
deps = {k.format(link_root_name = link_root_name): v for k, v in deps.items()}
Copy link
Member

Choose a reason for hiding this comment

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

It would probably be good to avoid .items() create giant new arrays?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i'd love your opinion here. the problem is that link_root_name is not known at the time these dicts are constructed, only later. is it possible to compute link_root_name in the module extension and fully materialize these?

alternately, we could defer this expansion until the point where the deps are actually used, but i wasn't sure if there was a nice single point for that or not.

I figured you know this better, wdyt?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

my preference would be to fully materialize at extension time if that's possible

Copy link
Member

Choose a reason for hiding this comment

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

Ideally it could all be avoided until later.

Is this where dropping the link_root_name param would help?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok I will try to trace it and see if we can defer, but yeah if we can drop lonk_root_name this becomes a non-issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbedard jbedard jbedard left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dzbarsky @jbedard