Skip to content

Conversation

@HunterLarco
Copy link

@HunterLarco HunterLarco commented Nov 30, 2025

Changes the rollup rule to expand location expressions in user-supplied args.

For example, as if this PR one could:

rollup(
  name = "example",
  args = ["--configCustomField", "$(location :foo)"],
  srcs = [":foo"],
)

js_library(
  name = "foo",
  srcs = ["foo.js"],
)

Typical usages of rollup will not need this feature and will remain unaffected, however, rollup does support custom command line arguments and in those cases it can be useful to pass a build-time data file. Location expansion allows users to express this without hardcoding bazel file structures (see the above example).


Changes are visible to end-users: yes

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

Test plan

I'm happy to add new tests, just let me know. The existing tests don't appear to check the contents of the bundle which would be necessary to fully test this feature. That said it's relatively straightforward so I could see this not requiring new tests.

@CLAassistant
Copy link

CLAassistant commented Nov 30, 2025

CLA assistant check
All committers have signed the CLA.

@aspect-workflows
Copy link

aspect-workflows bot commented Nov 30, 2025

Test

All tests were cache hits

11 tests (100.0%) were fully cached saving 918ms.


Test

e2e/smoke

All tests were cache hits

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


Buildifier

@HunterLarco HunterLarco marked this pull request as ready for review November 30, 2025 18:21

# Add user specified arguments *before* rule supplied arguments
args.add_all(ctx.attr.args)
args.add_all([ctx.expand_location(args) for arg in ctx.attr.args])
Copy link
Member

Choose a reason for hiding this comment

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

I think arg (not args) should be passed to expand_location?

@jbedard
Copy link
Member

jbedard commented Dec 6, 2025

See the write_source_files tests that are asserting the output is what's expected, I think you'd do the same to test this.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants