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.

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

bazel: remove rules_cc #19760

Merged
merged 2 commits into from
Apr 27, 2022
Merged

bazel: remove rules_cc #19760

merged 2 commits into from
Apr 27, 2022

Conversation

keith
Copy link
Member

@keith keith commented Jan 31, 2022

The plans for bazel to move to rules_cc have been postponed without any
communication. There's no value to us in using this right now, but it
will be trivial to re-adopt in the future if needed. But it has the
downside of using a fork of bazel's crosstool, that has to be updated
independently of bazel, which doesn't always happen as improvements are
made.

More details: bazelbuild/rules_cc#86 bazelbuild/bazel#14150

This also required a --host_action_env addition to mirror the variables we
pass through to actions in general. This is required because C++ toolchain
setup which discovers linkers in the cc configuration which uses PATH directly,
but then host actions didn't use PATH because of
--incompatible_strict_action_env, so gcc couldn't discover the path of lld even
though -fuse-ld=lld was passed.

Fixes #16608

Signed-off-by: Keith Smiley keithbsmiley@gmail.com

@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Jan 31, 2022
@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 @moderation

🐱

Caused by: #19760 was opened by keith.

see: more, trace.

@keith
Copy link
Member Author

keith commented Jan 31, 2022

Hoping we're good here now that we're on 5.x, because we were waiting for some upstream change there

@phlax
Copy link
Member

phlax commented Feb 7, 2022

this is breaking due to gcc linking issues

ERROR: /build/tmp/_bazel_envoybuild/b570b5ccd0454dc9af9f65ab1833764d/external/com_google_protobuf/BUILD:513:10: Linking external/com_google_protobuf/protoc failed: (Exit 1): gcc failed: error executing command 

not sure of the exact cause, but as discussed offline i think removing rules_cc is breaking the gcc toolchain as we were seeing a similar symptom during the bazel 5 upgrade

@keith keith marked this pull request as draft March 9, 2022 01:51
@keith keith force-pushed the ks/bazel-remove-rules_cc branch 2 times, most recently from 5dea829 to 0de2211 Compare March 24, 2022 23:54
@keith
Copy link
Member Author

keith commented Apr 2, 2022

I think I got to the bottom of that and updated the description accordingly, we'll see how CI feels

@keith
Copy link
Member Author

keith commented Apr 19, 2022

Depends on #20814

The plans for bazel to move to rules_cc have been postponed without any
communication. There's no value to us in using this right now, but it
will be trivial to re-adopt in the future if needed. But it has the
downside of using a fork of bazel's crosstool, that has to be updated
independently of bazel, which doesn't always happen as improvements are
made.

More details: bazelbuild/rules_cc#86 bazelbuild/bazel#14150

Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
This is for the same purpose as action_env except for host actions. This
is required for C++ toolchain setup which discovers linkers in the cc
configuration which uses PATH, but then host actions didn't use PATH
because of --incompatible_strict_action_env, so gcc couldn't discover
the `-fuse-ld=lld` path that was setup during the configuration.

Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
@keith
Copy link
Member Author

keith commented Apr 21, 2022

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #19760 (comment) was created by @keith.

see: more, trace.

@keith
Copy link
Member Author

keith commented Apr 22, 2022

/retest

@repokitteh-read-only
Copy link

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

🐱

Caused by: a #19760 (comment) was created by @keith.

see: more, trace.

@keith keith marked this pull request as ready for review April 22, 2022 04:35
@keith
Copy link
Member Author

keith commented Apr 22, 2022

Ok this is ready for review! The key here was to update buildifier to no longer try to enforce using rules_cc (they used to but flipped the default since things have stalled), and to pass through the host_action_env to make sure the setup uses the right tools

@keith keith enabled auto-merge (squash) April 22, 2022 04:36
@keith keith disabled auto-merge April 22, 2022 04:36
@keith keith enabled auto-merge (squash) April 22, 2022 04:36
@moderation
Copy link
Contributor

Always like deps removal 👍

/lgtm deps

@repokitteh-read-only repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Apr 22, 2022
@keith
Copy link
Member Author

keith commented Apr 26, 2022

bump

keith added a commit to envoyproxy/envoy-mobile that referenced this pull request Apr 26, 2022
Both of these projects are stalled on the bazel side, and these just
pass through to the native rules anyways. I'm removing rules_cc from
envoy as well envoyproxy/envoy#19760 so this
mirrors that.

Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
keith added a commit to envoyproxy/envoy-mobile that referenced this pull request Apr 26, 2022
Both of these projects are stalled on the bazel side, and these just
pass through to the native rules anyways. I'm removing rules_cc from
envoy as well envoyproxy/envoy#19760 so this
mirrors that.

Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
keith added a commit to envoyproxy/envoy-mobile that referenced this pull request Apr 26, 2022
Both of these projects are stalled on the bazel side, and these just
pass through to the native rules anyways. I'm removing rules_cc from
envoy as well envoyproxy/envoy#19760 so this
mirrors that.

Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
@phlax phlax requested a review from htuch April 26, 2022 16:20
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.

LGTM, thanks!

@keith keith merged commit 1edd65f into envoyproxy:main Apr 27, 2022
@keith keith deleted the ks/bazel-remove-rules_cc branch April 27, 2022 04:16
@adisuissa
Copy link
Contributor

Following this PR, a few of us have started seeing differences between running the check_format.py tool with docker and without.

When running check_format using docker (ci/run_envoy_docker.sh './ci/do_ci.sh check_format'), it runs well and doesn't complain.
When running without docker (tools/code_format/check_format.py check), it will complain about BUILD files which are missing some definitions. For example, it will complain that the file "source/common/protobuf/BUILD" is missing cc_proto_library as it is being used.

It seems that after this PR the cc_proto_library definition is missing from the file. Should this be reverted?
@phlax can you think why this PR could show different results when running with and without docker?

cc @jmarantz @sergiitk

@keith
Copy link
Member Author

keith commented Apr 29, 2022

Can you update buildifier locally? There was a default flip that we're relying on that I update CI for. I can probably also bake that into the script to avoid this issue for older versions. Ideally we would start vendoring buildifier to avoid this.

@adisuissa
Copy link
Contributor

Upgrading buildifier fixed this, thanks!

keith added a commit to envoyproxy/envoy-mobile that referenced this pull request Apr 30, 2022
Both of these projects are stalled on the bazel side, and these just
pass through to the native rules anyways. I'm removing rules_cc from
envoy as well envoyproxy/envoy#19760 so this
mirrors that.

Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
keith added a commit to envoyproxy/envoy-mobile that referenced this pull request May 9, 2022
Both of these projects are stalled on the bazel side, and these just
pass through to the native rules anyways. I'm removing rules_cc from
envoy as well envoyproxy/envoy#19760 so this
mirrors that.

Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
keith added a commit to envoyproxy/envoy-mobile that referenced this pull request May 9, 2022
Both of these projects are stalled on the bazel side, and these just
pass through to the native rules anyways. I'm removing rules_cc from
envoy as well envoyproxy/envoy#19760 so this
mirrors that.

Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
ravenblackx pushed a commit to ravenblackx/envoy that referenced this pull request Jun 8, 2022
The plans for bazel to move to rules_cc have been postponed without any
communication. There's no value to us in using this right now, but it
will be trivial to re-adopt in the future if needed. But it has the
downside of using a fork of bazel's crosstool, that has to be updated
independently of bazel, which doesn't always happen as improvements are
made.

More details: bazelbuild/rules_cc#86 bazelbuild/bazel#14150

This also required a `--host_action_env` addition to mirror the variables we
pass through to actions in general. This is required because C++ toolchain
setup which discovers linkers in the cc configuration which uses PATH directly,
but then host actions didn't use PATH because of
--incompatible_strict_action_env, so gcc couldn't discover the path of lld even
though `-fuse-ld=lld` was passed.

Fixes envoyproxy#16608

Signed-off-by: Keith Smiley <keithbsmiley@gmail.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.

the libc++ build configuration does not work on recent clang versions
6 participants