tools: setup build in local_fix_format

[ohadvano]

Commit Message: tools: setup build in local_fix_format
Additional Description: I am trying to reduce the time it takes to check and fix only the code format, without further check such as examples and docs. The problem I'm facing is that when running ./ci/run_envoy_docker.sh './tools/local_fix_format.sh -all', clang is not configured. Calling local_fix_format.sh is an alternative to add another target in do_ci.sh, as suggested by @phlax.

Release Notes: None
Platform Specific Features: None

[phlax]

thanks for addressing this @ohadvano

as discussed offline i think it would be better to make this configurable

cc @jmarantz

[jmarantz]

I'm not sure what this change does but the whole point of naming this local_fix_format was to use the local tools installed on your OS rather than using Docker.

To do this of course you need the right version of clang installed.

[phlax]

I'm not sure what this change does but the whole point of naming this local_fix_format was to use the local tools installed on your OS rather than using Docker.

it also has the benefit of being usable inside the container to only run this script, which was what @ohadvano wanted to do

i think with the if clause this should work equally well in both situations

[phlax]

lgtm, thanks @ohadvano

[phlax]

@jmarantz ill leave it to you to approve but i think this change is a good idea

[ohadvano]

I'm not sure what this change does but the whole point of naming this local_fix_format was to use the local tools installed on your OS rather than using Docker.

@jmarantz, I am running this locally, only I'm running this using docker (through ./ci/run_envoy_docker.sh).
I am not sure why, but in my Windows with WSL setup whenever I try to run ./tools/local_fix_format.sh directly without the Docker wrapper, I get the following error:

ERROR: Command clang-format-14 not found. If you have clang-format in version 12.x.x installed, but the binary name is different or it's not available in PATH, please use CLANG_FORMAT environment variable to specify the path. Examples:
    export CLANG_FORMAT=clang-format-14.0.0
    export CLANG_FORMAT=/opt/bin/clang-format-14
    export CLANG_FORMAT=/usr/local/opt/llvm@14/bin/clang-format

Although I'm pretty sure I have clang installed (otherwise couldn't have built envoy), tried exporting these env vars and it didn't help. Anyways, to your question - I do intend on running this locally.

[jmarantz]

Can you try to set these environment variables and run directly?

export CLANG_TOOLCHAIN=$HOME/ext/clang+llvm-14.0.0-x86_64-linux-gnu-ubuntu-18.04
export CLANG_FORMAT="$CLANG_TOOLCHAIN/bin/clang-format"
export BUILDIFIER_BIN="$HOME/go/bin/buildifier"
export BUILDOZER_BIN="$HOME/go/bin/buildozer"

You'l obviously have to set them correctly for your envirnment.

It might be that if your dev env is Windows based you really need Docker. I hadn't tested that.

Another option is for you to run that setup script from a wrapper script customized for your environment. The 4 lines above are for my custom wrapper script.

[ohadvano]

Thanks @jmarantz but it didn't work out for me. I assume that if I dig enough into trying to figure this out, I'll find how to make it work without the docker script, but I just thought that it doesn't worth my time if I can use the run_envoy_docker.sh instead. I think this PR adds a simple workaround that's transparent to whoever's not setting BUILD_REASON.

[jmarantz]

Sure; the point I was making is that you could have a script you use to run this one inside docker. This script might simply be:

. ci/build_setup.sh
tools/local_fix_format.sh "$@"

rather than adding the special-case into the script you could just put that into your personal wrapper script. This is what I do to set up my environment to specify where to find the binaries.

[phlax]

Sure; the point I was making is that you could have a script you use to run this one inside docker. This script might simply be:

the reason for doing it this way is to avoid adding more scripts or other ci targets

i think the suggested change doesnt affect current use in any way, and makes the most sense in terms of making this script versatile - build_setup.sh does exactly what is needed (i think) so there is no need to add more scripts doing the same

[jmarantz]

I wasn't suggesting adding a new script to the repo. I was suggesting that the repo should contain the curated basic scripts that can be used compose a solution specific to a user, which is how I use local_fix_format.sh; there's nothing particular to my setup in there; I have that in a personal script as it contains the exact location of where I installed the right version of clang etc.

I'm OK with checking this in as is though. If we were to put every possible setup in there with if-statements it would start to get unwieldy and hard to reason about, which is why I'm putting up mild resistance to this direction :)

Does this make sense?

[ohadvano]

@jmarantz, although this issue hasn't been posted by other devs, I assume that the outcome would be the same for anyone trying this use case (running this through Docker). It could be useful not only for me, so I don't consider this as a solution for a specific user..

[jmarantz]

That's valid except the premise of what I was trying to do with this script was to run without docker. Maybe at least you could drop some comments that you were unable to do this and it may be needed in the case of Windows?

[ohadvano]

@jmarantz of course. Added a comment.

[jmarantz]

Looking better. Now that it's a complete solution it's 11 lines including comments, you could argue it deserves to be its own independent script.

But I'm OK with this once we fix these nits.

[ohadvano]

/retest

[repokitteh-read-only]

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #27060 (comment) was created by @ohadvano.

see: more, trace.

[ohadvano]

@jmarantz I added a link to 'about WSL' in the comment

[ohadvano]

/retest

[repokitteh-read-only]

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #27060 (comment) was created by @ohadvano.

see: more, trace.