Skip to content

check_format: use GOPATH and add install commands for formatting libraries#2608

Merged
htuch merged 6 commits intoenvoyproxy:masterfrom
cmluciano:cml/formatdocs
Feb 16, 2018
Merged

check_format: use GOPATH and add install commands for formatting libraries#2608
htuch merged 6 commits intoenvoyproxy:masterfrom
cmluciano:cml/formatdocs

Conversation

@cmluciano
Copy link
Member

title: check_format: use GOPATH and add install commands for formatting libraries

Description:
The buildifier docs recommend using the go toolchain to install buildifier. I'm not sure if current default just reflects the original authors GOPATH.

I added explicit installation instructions else ./tools/check_format.sh may fail for a new contributor with errors that seemingly indicate an actual format error.

Risk Level: Low

Testing:
passing locally with ./tools/check_format.sh check

Christopher M. Luciano added 2 commits February 14, 2018 13:09
These tools are referenced in various doc locations and unless
you specify the -5.0 you likely will end up with the wrong
clang-format library.

Signed-off-by: Christopher M. Luciano <cmluciano@us.ibm.com>
The buildifier docs recommend using the go toolchain to install
buildifier.

Signed-off-by: Christopher M. Luciano <cmluciano@us.ibm.com>
bazel/README.md Outdated
4. `bazel fetch //source/...` to fetch and build all external dependencies. This may take some time.
5. `bazel build //source/exe:envoy-static` from the Envoy source directory.
and also for [Buildifer](https://github.com/bazelbuild/buildtools) which is used for formatting bazel BUILD files.
4. `go get go get github.com/bazelbuild/buildtools/buildifier` to install buildifier
Copy link
Contributor

Choose a reason for hiding this comment

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

repeated go get

@danielhochman
Copy link
Contributor

nice. i was lazy and symlinked buildifier into /usr/lib/go when i ran into this issue, thanks for fixing.

Signed-off-by: Christopher M. Luciano <cmluciano@us.ibm.com>
@cmluciano
Copy link
Member Author

@danielhochman :P looks like this was also being leveraged by a container somewhere, looking where now

Christopher M. Luciano added 2 commits February 14, 2018 14:14
Signed-off-by: Christopher M. Luciano <cmluciano@us.ibm.com>
BUILDIFIER_BIN is used within several shell scripts already and the
default should work if not set.

Signed-off-by: Christopher M. Luciano <cmluciano@us.ibm.com>
@htuch
Copy link
Member

htuch commented Feb 15, 2018

Please merge master to pick up #2613, this should fix CI TSAN.

@cmluciano
Copy link
Member Author

ok fingers crossed on merging of master

4. `bazel fetch //source/...` to fetch and build all external dependencies. This may take some time.
5. `bazel build //source/exe:envoy-static` from the Envoy source directory.
and also for [Buildifer](https://github.com/bazelbuild/buildtools) which is used for formatting bazel BUILD files.
4. `go get github.com/bazelbuild/buildtools/buildifier` to install buildifier
Copy link
Member

Choose a reason for hiding this comment

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

Why only on Apple?

Copy link
Member Author

Choose a reason for hiding this comment

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

Should we split the OS stuff out into a unique heading?

Copy link
Member

Choose a reason for hiding this comment

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

Never mind, I was reading this incorrectly via the GH diffs, looks good.

@htuch htuch self-assigned this Feb 15, 2018
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Thanks!

4. `bazel fetch //source/...` to fetch and build all external dependencies. This may take some time.
5. `bazel build //source/exe:envoy-static` from the Envoy source directory.
and also for [Buildifer](https://github.com/bazelbuild/buildtools) which is used for formatting bazel BUILD files.
4. `go get github.com/bazelbuild/buildtools/buildifier` to install buildifier
Copy link
Member

Choose a reason for hiding this comment

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

Never mind, I was reading this incorrectly via the GH diffs, looks good.

@htuch htuch merged commit 0ab49ee into envoyproxy:master Feb 16, 2018
jpsim pushed a commit that referenced this pull request Nov 28, 2022
Description: Update Cronvoy config so it picks up the new defaults. Fixes envoyproxy/envoy-mobile#2600.
Risk Level: Low
Testing: existing tests
Docs Changes: n/a
Release Notes: n/a

Signed-off-by: Renjie Tang <renjietang@chromium.org>
Signed-off-by: JP Simard <jp@jpsim.com>
jpsim pushed a commit that referenced this pull request Nov 29, 2022
Description: Update Cronvoy config so it picks up the new defaults. Fixes envoyproxy/envoy-mobile#2600.
Risk Level: Low
Testing: existing tests
Docs Changes: n/a
Release Notes: n/a

Signed-off-by: Renjie Tang <renjietang@chromium.org>
Signed-off-by: JP Simard <jp@jpsim.com>
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