ci: force install/link bazelisk package in macos dependency setup #10357

sunjayBhatia · 2020-03-12T14:48:45Z

Description: Latest mac os images have bazel pre-installed so force install bazelisk and link with overwrite to allow install to succeed
Risk Level: Low
Testing: N/A
Docs Changes: N/A
Release Notes: N/A

Co-authored-by: Sunjay Bhatia <[email protected]> Co-authored-by: William A Rowe Jr <[email protected]> Signed-off-by: Sunjay Bhatia <[email protected]> Signed-off-by: William A Rowe Jr <[email protected]>

sunjayBhatia · 2020-03-12T14:58:26Z

According to this PR that is pending merge, this is the set of tools installed in the latest image we are using: actions/runner-images#538 (notice bazel, bazelisk, and cmake are already installed, the former two likely not via the same homebrew tap we are)

sunjayBhatia · 2020-03-12T15:05:18Z

Another solution is to just take whatever bazelisk comes with the image, thoughts? (though it is not linked to bazel on the PATH I believe so this might be more complicated)

Co-authored-by: Sunjay Bhatia <[email protected]> Co-authored-by: William A Rowe Jr <[email protected]> Signed-off-by: Sunjay Bhatia <[email protected]> Signed-off-by: William A Rowe Jr <[email protected]>

junr03 · 2020-03-12T17:31:27Z

ci/mac_ci_setup.sh

-    then
-        echo "Failed to install $1"
+    brew install "$1"
+    if ! brew link --overwrite "$1"; then


Is this necessary if we are unlinking below?

this overwrite was added in order to overwrite existing bazelisk symlinks that homebrew complained about (see the output here before the forceful link: https://dev.azure.com/cncf/envoy/_build/results?buildId=33967&view=logs&j=a5e52b91-c83f-5429-4a68-c246fc63a4f7&t=7d499b5d-1d92-5096-7919-3c7f8065da78&l=619), this change could potentially actually supercede the unlink of bazel so this could be the only change needed (will try it out)

So, both are required, see this build: https://dev.azure.com/cncf/envoy/_build/results?buildId=33982&view=logs&j=a5e52b91-c83f-5429-4a68-c246fc63a4f7&t=7d499b5d-1d92-5096-7919-3c7f8065da78&l=619

We could do a brew install --force on all installs to maybe not have to unlink bazel, though that may have unintended consequences?

it should work, i'll leave it with the --force unless theres any objections

@sunjayBhatia sorry I didn't catch that bazelisk was already installed. Per your comment above, why can't we just drop all the bazel related installs in this file?

@junr03 We totally could, but I believe brew install bazelisk downloads something from somewhere different than brew install bazelbuild/tap/bazelisk (which is possibly why our script tries to install bazelbuild/tap/bazelisk in the first place, since there is a check if the packages we want are already installed). If we're cool with staying with what is in the CI image by default that works. However, I don't think if we do that the symlink for bazel will actually be the bazelisk executable, so we would need to do some manual overriding of symlinks in that case.

We could also ask the maintainers of the image to install bazelisk (and bazel) via the expected upstream tap so this isn't an issue for us as well, seems like at least bazelisk is not installed in the image via bazelbuild/tap/bazelisk

However, I don't think if we do that the symlink for bazel will actually be the bazelisk executable.

I see, and we would end up using the version of bazel installed in the image instead of the one we would like via bazelisk and .bazelversion.

My overall opinion for this PR is to take bazelisk install out of the normal flow and do what we need to do for bazel/bazelisk separately from the other dep installs. I just don't think we should change how we install all the deps. Now whether this means making sure that bazel is correctly symlinked to bazelisk, or using brew, up to you.

Hopefully the bazelisk link --overwrite should take care of it Co-authored-by: Sunjay Bhatia <[email protected]> Co-authored-by: William A Rowe Jr <[email protected]> Signed-off-by: Sunjay Bhatia <[email protected]> Signed-off-by: William A Rowe Jr <[email protected]>

So bazelisk can overwrite pre-installed bazel Co-authored-by: Sunjay Bhatia <[email protected]> Co-authored-by: William A Rowe Jr <[email protected]> Signed-off-by: Sunjay Bhatia <[email protected]> Signed-off-by: William A Rowe Jr <[email protected]>

- use the bazelisk basic package that is installed already, the check to see if it is installed should always be true so this is like an assertion that bazel is present - unlink bazel - make a link ourselves linking bazelisk to bazel Co-authored-by: Sunjay Bhatia <[email protected]> Co-authored-by: William A Rowe Jr <[email protected]> Signed-off-by: Sunjay Bhatia <[email protected]> Signed-off-by: William A Rowe Jr <[email protected]>

just get the path and assert it exists Signed-off-by: Sunjay Bhatia <[email protected]>

lizan · 2020-03-12T22:00:33Z

/azp run envoy-macos

azure-pipelines · 2020-03-12T22:00:40Z

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

lizan · 2020-03-12T22:02:54Z

/azp run envoy-macos

azure-pipelines · 2020-03-12T22:03:02Z

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Sunjay Bhatia <[email protected]>

danzh2010 · 2020-03-13T15:37:20Z

Is this PR causing the MacOS failure here: https://dev.azure.com/cncf/envoy/_build/results?buildId=34096&view=logs&j=a5e52b91-c83f-5429-4a68-c246fc63a4f7&t=7d499b5d-1d92-5096-7919-3c7f8065da78&l=707

sunjayBhatia · 2020-03-13T16:00:33Z

Is this PR causing the MacOS failure here: https://dev.azure.com/cncf/envoy/_build/results?buildId=34096&view=logs&j=a5e52b91-c83f-5429-4a68-c246fc63a4f7&t=7d499b5d-1d92-5096-7919-3c7f8065da78&l=707

There was no code change that is causing that issue to happen in CI, the bump to a new version of the macOS image with new installed software seems to be the cause, see: #10357 (comment)

This PR is to address that change

junr03 · 2020-03-13T16:49:16Z

Ok so it looks from the progression of your commits that:

bazelisk is installed but not in the Path.
So we need to force install
But do we need to overwrite the link if it wasn't on the path?
Now we are not deleting the bazel symlink, and re-symlinking after installing bazelisk. Are we sure that calls to bazel in our script are invoking bazelisk and not bazel?

cc @lizan for a second pair of eyes. I know this is a small change, but I want to make sure that we are using the version of bazel we advertise via bazelisk.

sunjayBhatia · 2020-03-13T17:24:39Z

bazelisk is installed but not in the Path.

true, however there are some other files symlinked (or otherwise) on the path that conflict with our new bazelisk install, which is why the brew link --overwrite is needed

So we need to force install

we could do a check for bazel already being installed and a brew unlink bazelisk/tap/bazel instead, doing a force install made it one less check/step

But do we need to overwrite the link if it wasn't on the path?

See the above and this example: https://dev.azure.com/cncf/envoy/_build/results?buildId=34087&view=logs&j=a5e52b91-c83f-5429-4a68-c246fc63a4f7&t=7d499b5d-1d92-5096-7919-3c7f8065da78&l=735 (bazelisk is linked to bazelisk.js, the install must be via some node modules method?)

Now we are not deleting the bazel symlink, and re-symlinking after installing bazelisk. Are we sure that calls to bazel in our script are invoking bazelisk and not bazel?

Yep, see: https://dev.azure.com/cncf/envoy/_build/results?buildId=34087&view=logs&j=a5e52b91-c83f-5429-4a68-c246fc63a4f7&t=5852bf5a-5a02-52f3-bee8-4fdc90cda9d0&l=22 (I believe invoking bazel causing a download of bazel means we're actually running bazelisk)

sunjayBhatia · 2020-03-13T17:28:35Z

bazelisk might be installed on the new image via npm, just found: https://github.com/bazelbuild/bazelisk#releases

junr03

Alright, I think you have crossed all the Ts and dotted all the Is. Thank you so much!

zuercher

I think this is fine. It installs bazelisk and then forces it to be linked as bazel.

Description: move mac ci setup to use envoy's script. I don't see much of a point in keeping a separate script here. For instance, envoyproxy/envoy#10357 updated to fix bazel after CI images broke. Instead of keeping up-to-date we can piggy back on efforts in upstream Envoy. Obviously, if diverging needs evolve we might need a second script in Envoy Mobile that calls the Envoy script, and does additional work. Risk Level: low, CI will let us know if I screwed up. Testing: CI Signed-off-by: Jose Nino <[email protected]>

…0357) Latest mac os images have bazel pre-installed so force install bazelisk and link with overwrite to allow install to succeed Risk Level: Low Testing: N/A Docs Changes: N/A Release Notes: N/A Co-authored-by: Sunjay Bhatia <[email protected]> Co-authored-by: William A Rowe Jr <[email protected]> Signed-off-by: Sunjay Bhatia <[email protected]> Signed-off-by: William A Rowe Jr <[email protected]> Signed-off-by: Lizan Zhou <[email protected]>

…voyproxy#10357) Latest mac os images have bazel pre-installed so force install bazelisk and link with overwrite to allow install to succeed Risk Level: Low Testing: N/A Docs Changes: N/A Release Notes: N/A Co-authored-by: Sunjay Bhatia <[email protected]> Co-authored-by: William A Rowe Jr <[email protected]> Signed-off-by: Sunjay Bhatia <[email protected]> Signed-off-by: William A Rowe Jr <[email protected]> Signed-off-by: Lizan Zhou <[email protected]>

Description: move mac ci setup to use envoy's script. I don't see much of a point in keeping a separate script here. For instance, #10357 updated to fix bazel after CI images broke. Instead of keeping up-to-date we can piggy back on efforts in upstream Envoy. Obviously, if diverging needs evolve we might need a second script in Envoy Mobile that calls the Envoy script, and does additional work. Risk Level: low, CI will let us know if I screwed up. Testing: CI Signed-off-by: Jose Nino <[email protected]> Signed-off-by: JP Simard <[email protected]>

sunjayBhatia and others added 2 commits March 12, 2020 10:45

Unlink bazel so we can install bazelisk

360ce83

Co-authored-by: Sunjay Bhatia <[email protected]> Co-authored-by: William A Rowe Jr <[email protected]> Signed-off-by: Sunjay Bhatia <[email protected]> Signed-off-by: William A Rowe Jr <[email protected]>

Add note with link to installed software list

13792d7

Co-authored-by: Sunjay Bhatia <[email protected]> Co-authored-by: William A Rowe Jr <[email protected]> Signed-off-by: Sunjay Bhatia <[email protected]> Signed-off-by: William A Rowe Jr <[email protected]>

sunjayBhatia and others added 2 commits March 12, 2020 11:15

Do a forceful link overwrite for all packages we install

4b5b6e0

Co-authored-by: Sunjay Bhatia <[email protected]> Co-authored-by: William A Rowe Jr <[email protected]> Signed-off-by: Sunjay Bhatia <[email protected]> Signed-off-by: William A Rowe Jr <[email protected]>

Kick CI

2c37276

Co-authored-by: Sunjay Bhatia <[email protected]> Co-authored-by: William A Rowe Jr <[email protected]> Signed-off-by: Sunjay Bhatia <[email protected]> Signed-off-by: William A Rowe Jr <[email protected]>

mattklein123 assigned zuercher Mar 12, 2020

sunjayBhatia mentioned this pull request Mar 12, 2020

Enable Windows CI //source/exe:envoy-static build #10293

Merged

junr03 reviewed Mar 12, 2020

View reviewed changes

wrowe and others added 2 commits March 12, 2020 13:56

Force install homebrew packages

ef5c604

So bazelisk can overwrite pre-installed bazel Co-authored-by: Sunjay Bhatia <[email protected]> Co-authored-by: William A Rowe Jr <[email protected]> Signed-off-by: Sunjay Bhatia <[email protected]> Signed-off-by: William A Rowe Jr <[email protected]>

mattklein123 assigned junr03 Mar 12, 2020

sunjayBhatia changed the title ~~ci: Unlink bazel so we can install bazelisk~~ ci: force install/link homebrew packages so we can install bazelisk Mar 12, 2020

sunjayBhatia and others added 2 commits March 12, 2020 16:13

bazelisk is not installed via homebrew in the image

1ccc74c

just get the path and assert it exists Signed-off-by: Sunjay Bhatia <[email protected]>

sunjayBhatia added 2 commits March 13, 2020 08:50

ok, just force the bazelisk install since its not on the path

27e2d12

Signed-off-by: Sunjay Bhatia <[email protected]>

Add a message for debugging

908c044

Signed-off-by: Sunjay Bhatia <[email protected]>

sunjayBhatia changed the title ~~ci: force install/link homebrew packages so we can install bazelisk~~ ci: force install/link bazelisk package in macos dependency setup Mar 13, 2020

junr03 approved these changes Mar 13, 2020

View reviewed changes

zuercher approved these changes Mar 13, 2020

View reviewed changes

zuercher merged commit 660891e into envoyproxy:master Mar 13, 2020

sunjayBhatia deleted the fix-macos-bazelisk-dependency-install branch March 13, 2020 17:43

junr03 mentioned this pull request Mar 13, 2020

ci: update mac ci setup envoyproxy/envoy-mobile#750

Merged

junr03 mentioned this pull request May 15, 2020

macOS CI: fails to correctly link bazelisk #11210

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci: force install/link bazelisk package in macos dependency setup #10357

ci: force install/link bazelisk package in macos dependency setup #10357

sunjayBhatia commented Mar 12, 2020 •

edited

Loading

sunjayBhatia commented Mar 12, 2020 •

edited

Loading

sunjayBhatia commented Mar 12, 2020 •

edited

Loading

junr03 Mar 12, 2020

sunjayBhatia Mar 12, 2020

sunjayBhatia Mar 12, 2020

sunjayBhatia Mar 12, 2020

junr03 Mar 12, 2020

sunjayBhatia Mar 12, 2020 •

edited

Loading

sunjayBhatia Mar 12, 2020

junr03 Mar 12, 2020

lizan commented Mar 12, 2020

azure-pipelines bot commented Mar 12, 2020

lizan commented Mar 12, 2020

azure-pipelines bot commented Mar 12, 2020

danzh2010 commented Mar 13, 2020

sunjayBhatia commented Mar 13, 2020 •

edited

Loading

junr03 commented Mar 13, 2020

sunjayBhatia commented Mar 13, 2020 •

edited

Loading

sunjayBhatia commented Mar 13, 2020

junr03 left a comment

zuercher left a comment

ci: force install/link bazelisk package in macos dependency setup #10357

ci: force install/link bazelisk package in macos dependency setup #10357

Conversation

sunjayBhatia commented Mar 12, 2020 • edited Loading

sunjayBhatia commented Mar 12, 2020 • edited Loading

sunjayBhatia commented Mar 12, 2020 • edited Loading

junr03 Mar 12, 2020

Choose a reason for hiding this comment

sunjayBhatia Mar 12, 2020

Choose a reason for hiding this comment

sunjayBhatia Mar 12, 2020

Choose a reason for hiding this comment

sunjayBhatia Mar 12, 2020

Choose a reason for hiding this comment

junr03 Mar 12, 2020

Choose a reason for hiding this comment

sunjayBhatia Mar 12, 2020 • edited Loading

Choose a reason for hiding this comment

sunjayBhatia Mar 12, 2020

Choose a reason for hiding this comment

junr03 Mar 12, 2020

Choose a reason for hiding this comment

lizan commented Mar 12, 2020

azure-pipelines bot commented Mar 12, 2020

lizan commented Mar 12, 2020

azure-pipelines bot commented Mar 12, 2020

danzh2010 commented Mar 13, 2020

sunjayBhatia commented Mar 13, 2020 • edited Loading

junr03 commented Mar 13, 2020

sunjayBhatia commented Mar 13, 2020 • edited Loading

sunjayBhatia commented Mar 13, 2020

junr03 left a comment

Choose a reason for hiding this comment

zuercher left a comment

Choose a reason for hiding this comment

sunjayBhatia commented Mar 12, 2020 •

edited

Loading

sunjayBhatia commented Mar 12, 2020 •

edited

Loading

sunjayBhatia commented Mar 12, 2020 •

edited

Loading

sunjayBhatia Mar 12, 2020 •

edited

Loading

sunjayBhatia commented Mar 13, 2020 •

edited

Loading

sunjayBhatia commented Mar 13, 2020 •

edited

Loading