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

abseil-cpp: lock in provided vocabulary type #196718

Draft
wants to merge 5 commits into
base: staging
Choose a base branch
from

Conversation

tobim
Copy link
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

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.
@@ -26,6 +26,10 @@ stdenv.mkDerivation rec {
sha256 = "sha256-dEYMPWpa3J9EqtCq3kubdUYJivNRTOKUpNDx3UC1IcQ=";
};

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

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.

Copy link
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
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
Contributor Author

tobim commented Oct 20, 2022

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
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
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 🙈 .

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label May 21, 2023
@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants