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

Add direct include needed for absl::StrCat #36

Merged
merged 3 commits into from
Dec 21, 2023

Conversation

hershi
Copy link
Contributor

@hershi hershi commented Dec 17, 2023

Before this was used without a direct include statement, which relies on a transitive include to bring it into scope and can easily break... which it does with one of the more up-to-date versions of absl. This fix should help avoid breakages when we want to update the abseil dependency

Repro:

  • Add the following to the WORKSPACE
git_repository(
  name = "com_google_absl",
  remote = "https://github.com/abseil/abseil-cpp",
  commit = "27478af36914f18867ea32cb42db00dba15fffcd",
)
  • bazel build src/main/cc/any_sketch:any_sketch - errors out
    • After the fix, builds successfully

Before this was used without a direct include statement, which relies on
a transitive include to bring it into scope and can easily break...
which it does with one of the more up-to-date versions of absl. This fix
should help avoid breakages when we want to update the abseil
dependency

Repro:
* Add the following to the WORKSPACE
```
git_repository(
  name = "com_google_absl",
  remote = "https://github.com/abseil/abseil-cpp",
  commit = "27478af36914f18867ea32cb42db00dba15fffcd",
)
```
* bazel build src/main/cc/any_sketch:any_sketch - errors out
@wfa-reviewable
Copy link

This change is Reviewable

Guy Hershenbaum added 2 commits December 17, 2023 09:24
Before this was used without a direct include statement, which relies on
a transitive include to bring it into scope and can easily break...
which it does with one of the more up-to-date versions of absl. This fix
should help avoid breakages when we want to update the abseil
dependency

Repro:
* Add the following to the WORKSPACE
```
git_repository(
  name = "com_google_absl",
  remote = "https://github.com/abseil/abseil-cpp",
  commit = "27478af36914f18867ea32cb42db00dba15fffcd",
)
```
* bazel build src/main/cc/any_sketch:any_sketch - errors out
Copy link
Member

@SanjayVas SanjayVas left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @hershi)

a discussion (no related file):
Thank you for the contribution!


@hershi
Copy link
Contributor Author

hershi commented Dec 19, 2023

@SanjayVas is there any follow up action needed on me? Since I don't have write access I can't merge this :)

Also, lint seems to have failed due to an infra issue, not if it needs to be re-run and how

Error: ReserveCacheError: Unable to reserve cache with key ktlint-0.40.0, another job may be creating this cache.

@SanjayVas
Copy link
Member

@SanjayVas is there any follow up action needed on me? Since I don't have write access I can't merge this :)

I'll go ahead and merge.

Also, lint seems to have failed due to an infra issue, not if it needs to be re-run and how

Error: ReserveCacheError: Unable to reserve cache with key ktlint-0.40.0, another job may be creating this cache.

Re-run worked. I think this repo is using some old GitHub Action versions that have some flakiness. Will update in a separate PR.

@SanjayVas SanjayVas merged commit 89a22d5 into world-federation-of-advertisers:main Dec 21, 2023
5 checks passed
@hershi hershi deleted the StrCat_fix branch December 24, 2023 08:56
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