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

Remove dev_dependency from buildifier_prebuilt. #16428

Closed
wants to merge 1 commit into from

Conversation

criemen
Copy link
Collaborator

@criemen criemen commented May 5, 2024

Otherwise, including any targets from //:BUILD.bazel in something that gets invoked from the internal build system will result in a missing dependency error.
This is relevant for C#, where I add a file export to LICENSE to the top-level BUILD.bazel file.

Otherwise, including any targets from //:BUILD.bazel
in something that gets invoked from the internal build system
will result in a missing dependency error.
This is relevant for C#, where I add a file export
to `LICENSE` to the top-level `BUILD.bazel` file.
@criemen criemen marked this pull request as ready for review May 6, 2024 08:33
@criemen criemen requested a review from a team as a code owner May 6, 2024 08:33
@redsun82
Copy link
Contributor

redsun82 commented May 6, 2024

Annoying, but then I would rather put the buildifier target elsewhere where it doesn't cause such problems, rather than making the internal repo pull it in. Maybe if the buildifier target is moved into misc/bazel/BUILD.bazel it should be ok?

@criemen
Copy link
Collaborator Author

criemen commented May 6, 2024

That'd work, too! Does it still work if it's not placed in the top-level folder? I wasn't sure about this.

We also should/could duplicate the buildifier setup internally anyways, then we'd not "just" pull in this module for the external repo - after all, it's rather useful.

redsun82 added a commit that referenced this pull request May 6, 2024
See #16428 for details as to why
this is necessary.
@redsun82
Copy link
Contributor

redsun82 commented May 6, 2024

That'd work, too! Does it still work if it's not placed in the top-level folder? I wasn't sure about this.

It works, yes 🙂

We also should/could duplicate the buildifier setup internally anyways, then we'd not "just" pull in this module for the external repo - after all, it's rather useful.

In any case we'd need to add another explicit bazel_dep, as we can't use a module pulled in transitively. But it's true that then the dev dependency wouldn't be that useful.

@criemen
Copy link
Collaborator Author

criemen commented May 6, 2024

Closing in favor of #16435.

Yeah the second point was what I was thinking. Anyways, let's go with your solution for now!

@criemen criemen closed this May 6, 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.

None yet

2 participants