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: update rules_foreign_cc #18174

Merged
merged 1 commit into from
Sep 23, 2021

Conversation

keith
Copy link
Member

@keith keith commented Sep 18, 2021

This update is required for this commit
bazel-contrib/rules_foreign_cc@c41020e
in order to support Apple Silicon. Since our last update there have been
some breaking API changes. I followed these instructions to migrate:

https://github.com/bazelbuild/rules_foreign_cc/releases/tag/0.3.0
https://github.com/bazelbuild/rules_foreign_cc/releases/tag/0.4.0

This was reverted once because of a regression fixed by bazel-contrib/rules_foreign_cc@da8952e

It was reverted again because we saw a failure on envoy-mobile, but that issue turned out to be a configuration issue there, not the fault of this change. bazel-contrib/rules_foreign_cc#789 provides a better error for that case in the future, but isn't required

This reverts commit 7760bc0.

Signed-off-by: Keith Smiley [email protected]

@repokitteh-read-only
Copy link

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #18174 was opened by keith.

see: more, trace.

@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).

🐱

Caused by: #18174 was opened by keith.

see: more, trace.

@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Sep 18, 2021
@keith keith force-pushed the ks/bazel-update-rules_foreign_cc branch 2 times, most recently from 1bf9fa9 to 49b22d1 Compare September 21, 2021 21:51
@keith keith marked this pull request as ready for review September 21, 2021 23:50
@keith
Copy link
Member Author

keith commented Sep 21, 2021

Ok this is ready now, updated the description with the reasoning

Copy link
Contributor

@moderation moderation left a comment

Choose a reason for hiding this comment

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

@keith do you want to bump to the latest commit or do that in follow up?

This reverts commit 7760bc0.

Signed-off-by: Keith Smiley <[email protected]>
@keith keith force-pushed the ks/bazel-update-rules_foreign_cc branch from 49b22d1 to 9c10f8c Compare September 22, 2021 23:45
@keith keith requested a review from moderation September 22, 2021 23:46
@moderation
Copy link
Contributor

/lgtm deps

@repokitteh-read-only repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Sep 23, 2021
@htuch
Copy link
Member

htuch commented Sep 23, 2021

@keith there are so many reverts and reverts-of-reverts that my brain (and probably any other newcomer) is unable to process. Can you give a summary for posterity of how we got into the situation that we had so many reverts? What's the future for this?

@keith
Copy link
Member Author

keith commented Sep 23, 2021

I've updated the description with the original justification, as well as the revert history. The red CI appears to be unrelated

@keith
Copy link
Member Author

keith commented Sep 23, 2021

/retest

@repokitteh-read-only
Copy link

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

🐱

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

see: more, trace.

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! I think it would be good to have better early warning from envoy-mobile to address this as a post-commit action item.

Is it possible to have envoy-mobile as a non-blocking CI check?

@htuch htuch merged commit e48f000 into envoyproxy:main Sep 23, 2021
@keith keith deleted the ks/bazel-update-rules_foreign_cc branch September 23, 2021 20:24
@keith
Copy link
Member Author

keith commented Sep 24, 2021

totally, I submitted envoyproxy/envoy-mobile#1831 for discussion

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