Skip to content

abseil-cpp: lock in provided vocabulary type#196718

Draft
tobim wants to merge 5 commits intoNixOS:stagingfrom
tobim:pkgs/abseil-cpp/lock-vocabulary-type-impls
Draft

abseil-cpp: lock in provided vocabulary type#196718
tobim wants to merge 5 commits intoNixOS:stagingfrom
tobim:pkgs/abseil-cpp/lock-vocabulary-type-impls

Conversation

@tobim
Copy link
Copy Markdown
Contributor

@tobim tobim commented Oct 19, 2022

Description of changes

Abseil provides a special header that allows software distributions
to override compile option dependent features. If this file is left
as-is downstream packages will usually fail to link due to undefined
references.

See the upstream documentation for a more comprehensive explanation:
https://github.com/abseil/abseil-cpp/blob/lts_2021_11_02/absl/base/options.h

Up till now we allowed to bulid abseil itself with different C++
Standards to overcome this problem, but that approach didn't fully
solve it:

The override is an optional argument, so its easy to overlook.
Processes that see multiple variants of the library (becausse
they get pulled into the closure by different dependencies) are
subject to undefined behavior.
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.11 Release Notes (or backporting 22.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

cc @andersk @ilya-fedin @SuperSandro2000

tobim added 4 commits October 18, 2022 23:42
@tobim
abseil-cpp: lock in provided vocabulary types
78f9230 
Abseil provides a special header that allows software distributions
to override compile option dependent features. If this file is left
as-is downstream packages will usually fail to link due to undefined
references.

See the upstream documentation for a more comprehensive explanation:
https://github.com/abseil/abseil-cpp/blob/lts_2021_11_02/absl/base/options.h

Up till now we allowed to bulid abseil itself with different C++
Standards to overcome this problem, but that approach didn't fully
solve it:

 * The override is an optional argument, so its easy to overlook.
 * Processes that see multiple variants of the library (becausse
   they get pulled into the closure by different dependencies) are
   subject to undefined behavior.
@tobim
Remove remaining uses of cxxStandard
6e83610
@tobim
Remove unnecessary override
26021f5
@tobim
Fix or-tools with absl::string_view
d5395c4
@tobim tobim requested review from FRidh and jonringer as code owners October 19, 2022 07:08
@github-actions github-actions bot added the 6.topic: python Python is a high-level, general-purpose programming language. label Oct 19, 2022
@ofborg ofborg bot requested review from NickCao, andersk and ilya-fedin October 19, 2022 08:02
@ofborg ofborg bot added 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 501-1000 This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 1001-2500 This PR causes many rebuilds on Linux and should target the staging branches. labels Oct 19, 2022
ambroisie
ambroisie reviewed Oct 19, 2022
View reviewed changes
pkgs/development/libraries/science/math/or-tools/default.nix
};

patches = [
./absl-string_view.patch
Copy link
Copy Markdown
Contributor

@ambroisie ambroisie Oct 19, 2022

Choose a reason for hiding this comment

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

The patch should have a comment explaining its purpose.

SuperSandro2000
SuperSandro2000 approved these changes Oct 19, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

if this works, then it is a really great complexity reducer

@andersk
Copy link
Copy Markdown
Contributor

andersk commented Oct 19, 2022

This is a respin of #186490, and my comments there still apply.

Abseil supports multiple C++ standards versions, and downstream packages have conflicting requirements on this version. The fact that you needed to patch one is proof. There’s no reason to suspect we won’t need more such patches in the future if we try to deviate from upstream in this way.

If the goal is to prevent shared library conflicts at runtime, then we should allow cxxStandard to vary as long as static = true.

@tobim
Copy link
Copy Markdown
Contributor Author

tobim commented Oct 20, 2022 •
edited
Loading

Abseil supports multiple C++ standards versions, and downstream packages have conflicting requirements on this version. The fact that you needed to patch one is proof. There’s no reason to suspect we won’t need more such patches in the future if we try to deviate from upstream in this way.

  1. All of that is correct. But how do we solve the cases when different instantiations of the same abseil release are present in a package closure?

  2. The or-tools patch will be needed as long as we use the compatibility implementation of string_view.
    Basically every option we have comes with a drawback:

    • ABSL_OPTION_USE_STD_STRING_VIEW = 2: We allow setting cxxStandard and have multiple conflicting instances of abseil in nixpkgs, and packages that pull in multiple versions of abseil may break in unpredictable ways.
    • ABSL_OPTION_USE_STD_STRING_VIEW = 1: You are restricted to C++17 and later when using abseil in nixpkgs. (This is what Debain, Fedora, and Arch do)
    • ABSL_OPTION_USE_STD_STRING_VIEW = 0: We have to patch packages that use string_view in ways that are not supported by the compatibility implementation.

    To me the options with 0 and 1 seem acceptable, I chose 0 because I assumed it would cause fewer issues.

If the goal is to prevent shared library conflicts at runtime, then we should allow cxxStandard to vary as long as static = true.

This is still an issue with static linking, thankfully it should fail when linking a shared object or executable and not at runtime.

Edit: Corrected option values.

@andersk
Copy link
Copy Markdown
Contributor

andersk commented Oct 20, 2022

But how do we solve the cases when different instantiations of the same abseil release are present in a package closure?

Having them in the same package closure isn’t a problem. I can even load or-tools and tensorflow into the same Python process because the abseil-cpp in or-tools is statically linked.

Having them in the same link command would be a problem. Is that a problem we have? Seems to me the appropriate response is “don’t do that, then”—just like it would be if you tried to link both qt514 and qt515 or the many other pairs of incompatible libraries that exist for good reasons in Nixpkgs.

This is still an issue with static linking, thankfully it should fail when linking a shared object or executable and not at runtime.

If you write an incorrect derivation, a build failure sounds like the right result to me.

@tobim
Copy link
Copy Markdown
Contributor Author

tobim commented Oct 20, 2022

Having them in the same link command would be a problem. Is that a problem we have? Seems to me the appropriate response is “don’t do that, then”—just like it would be if you tried to link both qt514 and qt515 or the many other pairs of incompatible libraries that exist for good reasons in Nixpkgs.

I have that problem right now trying to build pkgsStatic.arrow-cpp 🙈 .

@applePrincess applePrincess added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 19, 2022
@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label May 21, 2023
This was referenced Aug 13, 2023
Merged
Merged
@acairncross acairncross mentioned this pull request Sep 16, 2023
Merged
12 tasks
@wegank wegank marked this pull request as draft March 20, 2024 15:53
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 4, 2024
@nixpkgs-ci nixpkgs-ci bot added the 12.approvals: 1 This PR was reviewed and approved by one person. label Jun 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ambroisie ambroisie ambroisie left review comments

@SuperSandro2000 SuperSandro2000 SuperSandro2000 approved these changes

@FRidh FRidh Awaiting requested review from FRidh

@jonringer jonringer Awaiting requested review from jonringer

@andersk andersk Awaiting requested review from andersk

@NickCao NickCao Awaiting requested review from NickCao

@ilya-fedin ilya-fedin Awaiting requested review from ilya-fedin

Assignees

No one assigned

Labels

2.status: merge conflict This PR has merge conflicts with the target branch 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: python Python is a high-level, general-purpose programming language. 10.rebuild-darwin: 501-1000 This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 1001-2500 This PR causes many rebuilds on Linux and should target the staging branches. 12.approvals: 1 This PR was reviewed and approved by one person.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@tobim @andersk @SuperSandro2000 @ambroisie @wegank @applePrincess