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

Rule to conveniently use buildozer #326

Closed
wants to merge 1 commit into from

Conversation

ash2k
Copy link
Contributor

@ash2k ash2k commented Jun 8, 2018

This is a follow up for #318.
Fixes #316.

@ash2k
Copy link
Contributor Author

ash2k commented Jun 8, 2018

@laurentlb

@laurentlb
Copy link
Contributor

What's the use-case for the Buildozer rule?

I expect users to call it bazel run --direct_run //:buildozer and pass arguments on the command-line. Any reason to always call Buildozer with the same set of arguments?

@ash2k
Copy link
Contributor Author

ash2k commented Jun 8, 2018

  1. I think it is more convenient to it use via a rule rather than as a binary.
  2. Also that way I don't have to worry about pulling in the needed dependencies (let Bazel pull correct/needed ones).

E.g. I have a Go project and I'm currently managing buildtools dependency as a code dependency. This is not right because it is not a code dependency and also it is not convenient. See https://github.com/atlassian/smith/blob/132ef927b105cb1efc5312eb5ba961239222e766/Gopkg.toml#L26-L40 and https://github.com/atlassian/smith/blob/3751c511eea56558b420d7d5f50b2a4a663b9f0c/Makefile#L22-L31. I'd like to replace this with two clean bazel run invocations. In fact, I did this already in some other repo using my fork:

bazel run --direct_run //:buildozer
bazel run --direct_run //:buildifier

@laurentlb
Copy link
Contributor

I'd prefer if we don't have to maintain this. The set of flags will evolve over time, and I don't think it's very useful to wrap the binary in a rule and list all flags there.

You can run it like this and specify the arguments on the command-line:
$ bazel run @com_github_bazelbuild_buildtools//buildozer -- my arguments

If you want everything wrapped, maybe you should put the code in your repository? In that case, it might be easier to write a small shell script instead of creating this rule.

@ash2k
Copy link
Contributor Author

ash2k commented Jun 12, 2018

bazel run @com_github_bazelbuild_buildtools//buildozer -- my arguments

I didn't know this is possible, thanks for the tip! That helps with the "bazel maintains the dependencies for tools" problem. However, I don't understand how this PR is different from #318? I think it makes sense to have maintenance tools for Bazel stuff available as runnable rules. Someone may have a genrule that produces buildozer commands and that would be more convenient to depend on its label rather than call bazel build + bazel run. I don't know, maybe it is not a great example. I just think invoking things from bazel via rules is more convenient than shell scripts.

As a side: why does bazel have the run command at all? Why not bazel build and then just run the binary? I think because of dependencies, right? Maybe they are part of the graph and are also produced by some build steps.

@laurentlb
Copy link
Contributor

#318 is a bit different. Typical use-case for Buildifier is to run it on every modified file before submitting the code (or your editor could do it when you save the file). So the script is convenient: anyone can run Buildifier easily over the depot. This can be part of the standard workflow, if a project decides it.

For Buildozer, you don't have this use-case. You would normally use it to apply a one-time transformation. Or you call it as part of a script, or something else.

@ash2k
Copy link
Contributor Author

ash2k commented Jun 12, 2018

The workflow you described for buildifier is the workflow we also use for buildozer. In our case this is to catch freshly added targets and alter their attributes (add race="on" to enable race detector in tests for rules_go, add some tags to go_image to exclude those targets from tests, and more). So we use both tools the same way.

@laurentlb
Copy link
Contributor

Thanks. I see how it's useful for you. It's not obvious to me that it will be useful to a large number of users (running Buildifier on every file is something we actively encourage), and that it's worth maintaining this rule (Buildifier interface is super simple and doesn't really change; Buildozer has more flags/features and I expect it to evolve).

My suggestion is that you keep the Buildozer rule in your codebase (if you want). We'll see how things evolve, and how many users ask us for this kind of rule.

@ash2k
Copy link
Contributor Author

ash2k commented Jun 13, 2018

Ok, np. Thank you.

@ash2k ash2k closed this Jun 13, 2018
@ash2k
Copy link
Contributor Author

ash2k commented Jun 13, 2018

You can run it like this and specify the arguments on the command-line:
$ bazel run @com_github_bazelbuild_buildtools//buildozer -- my arguments

Hm, I cannot actually run it like this because of the Bazel sandbox. I've just tried and it does not modify any files - I guess because it does not see them.

@ash2k
Copy link
Contributor Author

ash2k commented Jun 13, 2018

In my makefile I have to do this:

OS = $$(uname -s | tr A-Z a-z)
BINARY_PREFIX_DIRECTORY=$(OS)_amd64_stripped

.PHONY: fmt-bazel
fmt-bazel:
	bazel build @com_github_bazelbuild_buildtools//buildozer
	-bazel-bin/external/com_github_bazelbuild_buildtools/buildozer/$(BINARY_PREFIX_DIRECTORY)/buildozer \
		-f build/buildozer_commands.txt

And that way it works. Not awesome UX.

@ash2k ash2k reopened this Jun 13, 2018
@ash2k
Copy link
Contributor Author

ash2k commented Jun 15, 2018

For those who will find this via search - this rule is available here https://github.com/atlassian/bazel-tools/tree/master/buildozer and a migration example is here atlassian/smith#295

@ash2k ash2k closed this Jun 15, 2018
@ash2k ash2k deleted the buildozer_rule branch August 2, 2018 07:41
@ddlees
Copy link

ddlees commented Jan 24, 2020

It's a shame this was closed. It would have been great to have this kind of functionality come straight from the source but I suppose I can use the atlassian repository.

@michael-christen
Copy link

#387 (comment) is a pretty decent solution, fyi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow builifier and buildozer to be run via a rule
5 participants