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.

Sign up for GitHub

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

Jump to bottom

bazel: Use vendored tools from foreign_cc #27211

Merged
merged 2 commits into from
May 5, 2023
Merged

bazel: Use vendored tools from foreign_cc #27211

merged 2 commits into from
May 5, 2023

Conversation

phlax
Copy link
Member

@phlax phlax commented May 5, 2023 •
edited
Loading

The kitware apt repo is broken (again) which broke our CI and means we cant update the build images

Using host tools not only bloats the build image but makes builds a lot less deterministic/predictable

This revives prior art to try and achieve this and updates foreign_cc to latest

Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

@keith @phlax
bazel: Use vendored tools from foreign_cc
cb65fc9 
Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>

Signed-off-by: Ryan Northey <ryan@synca.io>
@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label May 5, 2023
@repokitteh-read-only
Copy link

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @mattklein123

🐱

Caused by: #27211 was opened by phlax.

see: more, trace.

phlax
phlax commented May 5, 2023
View reviewed changes
bazel/dependency_imports.bzl
@@ -23,8 +23,7 @@ JQ_VERSION = "1.6"
YQ_VERSION = "4.24.4"

def envoy_dependency_imports(go_version = GO_VERSION, jq_version = JQ_VERSION, yq_version = YQ_VERSION):
# TODO: allow building of tools for easier onboarding
rules_foreign_cc_dependencies(register_default_tools = False, register_built_tools = False)
rules_foreign_cc_dependencies()
Copy link
Member Author

Choose a reason for hiding this comment

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

@keith if this is still an issue for mac users can we add a select here

it would be really good to get rid of this out of the build image, currently we cant even update it

Copy link
Member

Choose a reason for hiding this comment

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

Yep I can try today

@phlax phlax mentioned this pull request May 5, 2023
Merged
@phlax
Copy link
Member Author

phlax commented May 5, 2023

not sure about this line

bazel/external/boringssl_fips.genrule_cmd:export PATH="$(dirname `which cmake`):/usr/bin:/bin"

altho i guess it should hopefully work the same

keith
keith reviewed May 5, 2023
View reviewed changes
Copy link
Member

@keith keith left a comment

Choose a reason for hiding this comment

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

Last I tried this we have a dep that uses a genrule that uses cmake and that failed

bazel/setup_clang.sh
build:clang --action_env='PATH=${PATH}' --host_action_env='PATH=${PATH}'
build:clang --action_env=CC=clang --host_action_env=CC=clang
build:clang --action_env=CXX=clang++ --host_action_env=CXX=clang++
build:clang --action_env='LLVM_CONFIG=${LLVM_PREFIX}/bin/llvm-config' --host_action_env='LLVM_CONFIG=${LLVM_PREFIX}/bin/llvm-config'
Copy link
Member

Choose a reason for hiding this comment

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

Probably need this one regardless of this change?

bazel/dependency_imports.bzl
@@ -23,8 +23,7 @@ JQ_VERSION = "1.6"
YQ_VERSION = "4.24.4"

def envoy_dependency_imports(go_version = GO_VERSION, jq_version = JQ_VERSION, yq_version = YQ_VERSION):
# TODO: allow building of tools for easier onboarding
rules_foreign_cc_dependencies(register_default_tools = False, register_built_tools = False)
rules_foreign_cc_dependencies()
Copy link
Member

Choose a reason for hiding this comment

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

Yep I can try today

@keith
Copy link
Member

keith commented May 5, 2023

previous chat on #20910

@phlax
Copy link
Member Author

phlax commented May 5, 2023

seems to have made it through ci (other than ~unrelated BEP fail)

@keith
Copy link
Member

keith commented May 5, 2023

does the CI image still have cmake? if so that one is likely being picked up instead. I only found it because I tried it in a macOS VM that didn't have anything installed

@phlax
Copy link
Member Author

phlax commented May 5, 2023

does the CI image still have cmake?

it does - let me add an rm -rf in the container entry to resolve that

@phlax
no-cmake
faf083b 
Signed-off-by: Ryan Northey <ryan@synca.io>
@keith
Copy link
Member

keith commented May 5, 2023

in my VM I see make actually crash, I saw this in my last attempt at this too, I'm not sure if it's unique to this setup somehow or not

image

@phlax
Copy link
Member Author

phlax commented May 5, 2023

seems to be passing ci - would be good to figure if this is related and/or cause

@phlax
Copy link
Member Author

phlax commented May 5, 2023

looks macish to me - lets just add selects - you probably want to use your host cmake - but mostly thats not hte case

@keith
Copy link
Member

keith commented May 5, 2023

I think ideally we would use these on host platforms for sure, we could try to use a newer make to see if that luckily fixes it

@keith
Copy link
Member

keith commented May 5, 2023

it worked outside of my VM (but also could have picked up my system installs potentially)

@phlax
Copy link
Member Author

phlax commented May 5, 2023

seems to be passing mobile ci - which i think is mostly mac-based, so im guessing above issue is env-specific (not sure if there is some other reason they are still using existing cmake in container)

@phlax
Copy link
Member Author

phlax commented May 5, 2023

which i think is mostly mac-based, so im guessing above issue is env-specific

not sure exactly how these are related - i know that most of the mobile ci uses mac hosts, and some of it at least, uses the build image (or a derivative)

i guess my concern is that we remove it from the build image (the goal) and that breaks mobile

@keith
Copy link
Member

keith commented May 5, 2023

macOS images aren't affected by the docker image, android might be (don't recall if it uses this one or another one)

@phlax
Copy link
Member Author

phlax commented May 5, 2023

android might be (don't recall if it uses this one or another one)

ah yeah, its the mobile deriv with the android ndk - so unrelated to mac builds

@phlax phlax merged commit f76cc4d into envoyproxy:main May 5, 2023
This was referenced May 6, 2023
Closed
Closed
@milton0825 milton0825 mentioned this pull request May 23, 2023
Closed
reskin89 pushed a commit to reskin89/envoy that referenced this pull request Jul 11, 2023
@phlax @keith @reskin89
bazel: Use vendored tools from foreign_cc (envoyproxy#27211)
7dc641e 
Signed-off-by: Ryan Northey <ryan@synca.io>
Co-authored-by: Keith Smiley <keithbsmiley@gmail.com>
Signed-off-by: Ryan Eskin <ryan.eskin89@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keith keith keith approved these changes

Assignees

@keith keith

@mattklein123 mattklein123

Labels
deps Approval required for changes to Envoy's external dependencies
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@phlax @keith @mattklein123