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

Prefer using system Abseil if available #347

Merged
merged 3 commits into from
Aug 26, 2024
Merged

Conversation

carlocab
Copy link
Contributor

@carlocab carlocab commented Jul 2, 2023

Newer versions of Protobuf (22+) pull in Abseil as a dependency. We
want to avoid using our bundled copy for these cases, as this will
likely conflict with the version of Abseil that Protobuf uses.

The simplest way to do this seems to be to just prefer a system
installation of Abseil if it's available.

I've also cherry-picked from #207 to simplify what I want to do here.

@carlocab
Copy link
Contributor Author

carlocab commented Jul 2, 2023

I've also noticed some similar logic gated behind if(UNIX OR MINGW), but I don't quite follow why this is done. Happy to require something similar behind using a system Abseil if desired.

carlocab added a commit to Homebrew/formula-patches that referenced this pull request Jul 2, 2023
This enables use of our Abseil in bloaty. This is a backport of the
patch upstreamed at google/bloaty#347.
@carlocab
Copy link
Contributor Author

Ping @haberman?

@haberman
Copy link
Member

haberman commented Apr 2, 2024

Sorry for the long delay on this.

The CI tests aren't running on this PR and I'm not sure why. They seems to be running on some PRs but not others.

The tests ran on the most recently merged PR #370. I wonder if there is any chance that PR broke the CI. Any idea @arunsathiya?

@arunsathiya
Copy link
Contributor

The tests ran on the most recently merged PR #370. I wonder if there is any chance that PR broke the CI. Any idea @arunsathiya?

I am not sure just yet, but the workflows on the PR #370 were manually triggered by you, as a maintainer of the project.

cifuzz.yml build.yml
image image

Would something similar be required here? I am unable to see those workflows on the checks section of the current PR #347, possibly because I am not a maintainer of this project:

image

@haberman
Copy link
Member

haberman commented Apr 4, 2024

I saw the button on the other PR and clicked it. But I don't see the button here. I am not sure why it disappeared.

@carlocab
Copy link
Contributor Author

I rebased on main. I think you can run the tests now.

@carlocab
Copy link
Contributor Author

The external Abseil build seems to choke at trying to build googletest. I've fixed that by pointing it at third_party/googletest.

Newer versions of Protobuf (22+) pull in Abseil as a dependency. We
want to avoid using our bundled copy for these cases, as this will
likely conflict with the version of Abseil that Protobuf uses.

The simplest way to do this seems to be to just prefer a system
installation of Abseil if it's available.

Signed-off-by: Carlo Cabrera <[email protected]>
@carlocab
Copy link
Contributor Author

The external Abseil build seems to choke at trying to build googletest. I've fixed that by pointing it at third_party/googletest.

Oops, that didn't work for some reason. I've just disabled Abseil's tests.

@carlocab
Copy link
Contributor Author

carlocab commented Aug 4, 2024

Ping @haberman?

If it helps, we've been using a version of this patch at Homebrew for over a year now1, and haven't gotten any complaints about it. (We've had at least ~5,000 installs in the past year.2 This count excludes users who opt out of Homebrew analytics.)

Footnotes

  1. https://github.com/Homebrew/homebrew-core/commit/be131c6c8ea3744a39d7fe059d2fb53cae464fc6

  2. https://formulae.brew.sh/formula/bloaty#default

@haberman haberman merged commit 6b78e08 into google:main Aug 26, 2024
1 check passed
@haberman
Copy link
Member

Unfortunately I think I need to roll this back for now, as it broke the continuous build: https://github.com/google/bloaty/actions/runs/10566910014/job/29274712202#step:10:315

/home/runner/work/bloaty/bloaty/bloaty/third_party/abseil-cpp/absl/debugging/failure_signal_handler.cc:139:32: error: no matching function for call to ‘max(long int, int)’
139 | size_t stack_size = (std::max(SIGSTKSZ, 65536) + page_mask) & ~page_mask;

We may need to update the version of ABSL first.

@haberman
Copy link
Member

Alternatively, would you be interested in trying to fix the continuous build with a fix-forward?

@carlocab
Copy link
Contributor Author

Alternatively, would you be interested in trying to fix the continuous build with a fix-forward?

Sure, let me look at trying to update the bundled copy of abseil.

@carlocab carlocab deleted the system-abseil branch August 27, 2024 06:47
@carlocab
Copy link
Contributor Author

#384

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