Skip to content
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

feat: allow write_source_file(s) to write source files to bazel packages outside of the target's package #717

Merged
merged 1 commit into from
Jan 10, 2024

Conversation

gregmagolan
Copy link
Collaborator

@gregmagolan gregmagolan commented Jan 10, 2024

We've received requests for this feature a few times now with the latest one being here https://bazelbuild.slack.com/archives/C04281DTLH0/p1704883631739649.

The glob trick to check that the out file exists must be disabled by setting check_that_out_file(s)_exists to False when writing to a different Bazel package. The trick doesn't work unless the out file is in the same Bazel package (glob's don't traverse into sub packages and can't look into sibling or parent packages).

Type of change

  • New feature or functionality (change which adds functionality)

For changes visible to end-users

  • Suggested release notes are provided below:

write_source_files(s) can now write source files to Bazel packages outside of the target's package when you set check_that_out_file(s)_exists to False.

Test plan

  • New test cases added

@gregmagolan gregmagolan force-pushed the write_source_files_out_file_different_package branch from c473cbd to 305ed64 Compare January 10, 2024 17:49
@gregmagolan gregmagolan changed the title write source files out file different package feat: allow write_source_file(s) to write source files to bazel packages outside of the target's package Jan 10, 2024
@gregmagolan gregmagolan force-pushed the write_source_files_out_file_different_package branch 2 times, most recently from 3362a9c to b1ef858 Compare January 10, 2024 18:00
@gregmagolan gregmagolan marked this pull request as ready for review January 10, 2024 18:28
@jbedard
Copy link
Collaborator

jbedard commented Jan 10, 2024

Will this not potentially add confusion to usage of this rule?

@gregmagolan
Copy link
Collaborator Author

gregmagolan commented Jan 10, 2024

Will this not potentially add confusion to usage of this rule?

The default behaviour is to not allow it and have the existence check so by default nothing changes. This allows some users to turn that check off and break the glass.

The docstring updates to out_file and check_that_out_file_exists now mention the opt-out. Do you have a suggestion for how to reduce the confusion?

@gregmagolan gregmagolan force-pushed the write_source_files_out_file_different_package branch from b1ef858 to 2c07db6 Compare January 10, 2024 19:19
@gregmagolan gregmagolan force-pushed the write_source_files_out_file_different_package branch 2 times, most recently from df0d490 to ddf5807 Compare January 10, 2024 19:31
@jbedard
Copy link
Collaborator

jbedard commented Jan 10, 2024

It's minor enough I'm fine with this. But imo docs explaining the restriction is better then expanding the api, adding docs explaining the restriction and docs explaining the api to workaround the restriction 🤷‍♂️

Can't users just move the write-source rule to the directory it's writing to?

@kormide
Copy link
Collaborator

kormide commented Jan 10, 2024

It's minor enough I'm fine with this. But imo docs explaining the restriction is better then expanding the api, adding docs explaining the restriction and docs explaining the api to workaround the restriction 🤷‍♂️

Can't users just move the write-source rule to the directory it's writing to?

I'm also curious about this. Does this ever solve a problem that can't be solved by just moving the rule invocation to another package?

What is the experience like when the rule is invoked but the file doesn't exist yet?

@gregmagolan
Copy link
Collaborator Author

It's minor enough I'm fine with this. But imo docs explaining the restriction is better then expanding the api, adding docs explaining the restriction and docs explaining the api to workaround the restriction 🤷‍♂️
Can't users just move the write-source rule to the directory it's writing to?

I'm also curious about this. Does this ever solve a problem that can't be solved by just moving the rule invocation to another package?

What is the experience like when the rule is invoked but the file doesn't exist yet?

You can always add another write_source_file(s) target in the Bazel package you're writing to. With check_that_out_file_exists off, the DX if the file is not there is degraded but that is mentioned in the docs and the developer adding a write_source_files target with the explicit check_that_out_file_exists opt-out so they can write to another Bazel package would know that and create the file so other developers wouldn't hit the degraded DX.

@gregmagolan gregmagolan force-pushed the write_source_files_out_file_different_package branch 6 times, most recently from fdf8698 to 4f9fa14 Compare January 10, 2024 22:47
@gregmagolan gregmagolan force-pushed the write_source_files_out_file_different_package branch from 4f9fa14 to c6205a8 Compare January 10, 2024 22:48
@gregmagolan gregmagolan merged commit 5b3b7d7 into main Jan 10, 2024
71 checks passed
@gregmagolan gregmagolan deleted the write_source_files_out_file_different_package branch January 10, 2024 23:07
gregmagolan added a commit that referenced this pull request Mar 27, 2024
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