Skip to content
This repository has been archived by the owner on Dec 15, 2021. It is now read-only.

Add yapf and clang-format to formatter #177

Closed
vmax opened this issue Jul 18, 2018 · 15 comments
Closed

Add yapf and clang-format to formatter #177

vmax opened this issue Jul 18, 2018 · 15 comments
Labels
good first issue Good for newcomers P2

Comments

@vmax
Copy link
Contributor

vmax commented Jul 18, 2018

//tools:formatter currently relies on yapf and clang-format being available on the user's system.
We should make a BUILD target that will make bazel download them, and then add them to runtime_deps of //tools:formatter, so that they're available to the tool to use.

@vmax vmax added the P2 label Jul 18, 2018
@oferb oferb changed the title Vendor-in yapf and clang-format Add yapf and clang-format to formatter (so we don't need to assume they're installed) Jul 18, 2018
@oferb oferb changed the title Add yapf and clang-format to formatter (so we don't need to assume they're installed) Add yapf and clang-format to formatter Jul 18, 2018
@oferb oferb added the good first issue Good for newcomers label Jul 18, 2018
@NikleshPhabiani
Copy link
Contributor

I would want to try out this one.

@oferb
Copy link
Contributor

oferb commented Jul 27, 2018

Ok, go ahead :)

@vmax
Copy link
Contributor Author

vmax commented Jul 29, 2018

@NikleshPhabiani for reference, you can have a look at how //tools:protoc is implemented. Remember it should work at both platforms (Linux/macOS). Ping me if you need any help 🙂

@NikleshPhabiani
Copy link
Contributor

NikleshPhabiani commented Jul 31, 2018

@vmax . Actually, I did happen to look at how //tools:protoc is implemented a few days back and looked something that would help me out in this issue but haven't looked at it lately. Thanks a lot for the update. Yes, will definitely let you know if I need any help.

@NikleshPhabiani
Copy link
Contributor

A snippet for installation of clang-format on macOS:

platform=$(uname)

if [ "$platform" == "Darwin" ]; then
  mkdir -p ./clang-format
  curl 'http://releases.llvm.org/6.0.0/clang+llvm-6.0.0-x86_64-apple-darwin.tar.xz' -o './clang-format/clang-format.tar.xz'
  tar xvfj clang-format/clang-format.tar.xz -C ./clang-format
  rm -f ~/bin/clang-format
  ln -s $(pwd)/$(find clang-format | grep bin/clang-format$) ~/bin/clang-format
fi

Reference Link

Is this fine or is something else expected? The other relevant files will be changed but this is just a snippet for installation on the system. Also, which binary from the releases needs to be used for installation on Linux? Thanks.

@oferb
Copy link
Contributor

oferb commented Aug 6, 2018

So the idea is to have bazel get the binary. That way no setup step is needed from the developer.
If you look at https://github.com/google/startup-os/blob/master/tools/BUILD#L36
It uses protoc_bin which is defined in WORKSPACE.

Ohh, and when we say linux, we actually mean Ubuntu :) so "Clang for Ubuntu 16.04".

@oferb
Copy link
Contributor

oferb commented Aug 9, 2018

Hey, how's it going?

@NikleshPhabiani
Copy link
Contributor

Hey. Sorry, couldn't work on this during the week. I will get this done early in the weekend. Oh ok, got it. Thanks a lot. One doubt. What would be the SHA-256 code for these? I couldn't figure out a way yet to retrieve a tag for each.

@oferb
Copy link
Contributor

oferb commented Aug 10, 2018

That's fine, just checking.
About SHA-256, we use this:
https://github.com/google/startup-os/blob/master/WORKSPACE#L1-L2
Thanks again!

@vmax
Copy link
Contributor Author

vmax commented Aug 11, 2018

@NikleshPhabiani One more SHA-256 tip: if you don't specify it, bazel will print it out on building the target that depends on that archive

@NikleshPhabiani
Copy link
Contributor

Thanks a lot for all the help. I have a doubt regarding YAPF. The release does not have separate versions for Linux and macOS. Is it platform independent and linked directly to Python? Kindly let me know what needs to be done with respect to YAPF.

@vmax
Copy link
Contributor Author

vmax commented Aug 19, 2018

@NikleshPhabiani it'll be great to use it as http_archive together with writing custom BUILD file with py_binary target in it

@oferb
Copy link
Contributor

oferb commented Sep 11, 2018

clang-format is now fully added. Thanks Niklesh! Do you want to continue this task for YAPF?
I believe that YAPF is platform-independent.

@NikleshPhabiani
Copy link
Contributor

NikleshPhabiani commented Sep 14, 2018

Actually, I am extremely tied up for sometime with university coursework and interviews. Hence, won't be able to pick this up atleast for a couple of days.

@oferb
Copy link
Contributor

oferb commented Sep 14, 2018

That's fine. I created an issue just for yapf #304
You're welcome to take a look, but just know that the python task is a P3 (lower priority), since we don't currently use python. There's other issues that are good first good issue (although you're not a first contributor anymore ;) you're welcome to take a look.

More details about P3 and other priorities: https://developers.google.com/issue-tracker/concepts/issues

@oferb oferb closed this as completed Sep 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
good first issue Good for newcomers P2
Projects
None yet
Development

No branches or pull requests

3 participants