abseil-cpp: lock in provided vocabulary types by tobim · Pull Request #186490 · NixOS/nixpkgs

tobim · 2022-08-13T20:04:27Z

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

I built up to libtensorflow without problem, but nixpkgs-review is too much for my hardware.
resolves #185865

SuperSandro2000 · 2022-08-14T00:04:10Z

There is at least another reference at pkgs/applications/networking/instant-messengers/telegram/kotatogram-desktop/default.nix:72:19.

tobim · 2022-08-14T05:39:01Z

There is at least another reference at pkgs/applications/networking/instant-messengers/telegram/kotatogram-desktop/default.nix:72:19.

Thanks, I removed the remaining referencces.

pkgs/applications/networking/instant-messengers/telegram/tdesktop/default.nix

pkgs/top-level/all-packages.nix

andersk

Part of the reason cxxStandard was introduced as an overridable option was to avoid breaking or-tools (#143967), and indeed, this PR breaks or-tools.

make run \
 SOURCE=examples/cpp/network_routing_sat.cc \
 ARGS="--clients=10 --backbones=5 --demands=10 --traffic_min=5 --traffic_max=10 --min_client_degree=2 --max_client_degree=5 --min_backbone_degree=3 --max_backbone_degree=5 --max_capacity=20 --fixed_charge_cost=10"
make[1]: Entering directory '/build/source'
which: no mvn in (/nix/store/s42gmjjg1gyg7f8s7nnjv0s3363f7gmx-cmake-3.23.2/bin:/nix/store/qq62j0jrq6h958gg0q8igyp7vbkaxclz-lsb_release/bin:/nix/store/azap5z6md0pqf8sxb1gjsyx5x5jdmzw4-swig-4.0.2/bin:/nix/store/ym72c0nn6m1v18r648c2r4fw7n2b5clk-which-2.21/bin:/nix/store/rc9cz7z4qlgmsbwvpw2acig5g2rdws46-python3-3.10.5/bin:/nix/store/g9hjv7rp76ga8majqkqqqb65m3wykbbr-python3.10-wheel-0.37.1/bin:/nix/store/2qg94y58v1jr4dw360bmpxlrs30m31ca-protobuf-3.19.4/bin:/nix/store/njzl18r4mc1b82jd172nlg1gs45z3g1h-patchelf-0.14.5/bin:/nix/store/kia8xygy2r9iblwbjrl163j4bpz84wz7-gcc-wrapper-11.3.0/bin:/nix/store/42izybv2fz3ps5pcg3dvcmn85pc9406y-gcc-11.3.0/bin:/nix/store/fg85dscd4408xg9wc12paxxnkp69k8ix-glibc-2.35-163-bin/bin:/nix/store/xp5z3k851fs7haqbcwqax1hh4pynzla9-coreutils-9.1/bin:/nix/store/klgmnpyyan0gg6ra6d40qmjqs39cj1cf-binutils-wrapper-2.38/bin:/nix/store/dpxy5cr7cm0nch0qvq2qygs9jwnmhiy5-binutils-2.38/bin:/nix/store/ir6kvyhfrpm20pqz3d7wxrbyrqd6ylkc-bzip2-1.0.8-bin/bin:/nix/store/dhj3jgjv3x9wycsckxrwhx7pd5ky81bw-mypy-protobuf-3.2.0/bin:/nix/store/xp5z3k851fs7haqbcwqax1hh4pynzla9-coreutils-9.1/bin:/nix/store/b1c0270m7fcppqpga49fqy7n8hj3vbw2-findutils-4.9.0/bin:/nix/store/73rp8gam5bpihafxbk3x39dza2iw3lzi-diffutils-3.8/bin:/nix/store/cr41h6cg093d7z6w0nnaiqpsfc2fxd54-gnused-4.8/bin:/nix/store/74lg75l5wdxkiab73pc7x3983lcppk86-gnugrep-3.7/bin:/nix/store/d7nppmazji1gcg1nmp6nvsp59ihxadyn-gawk-5.1.1/bin:/nix/store/j0h50va2scdc60gd20ib41awvl6dvfcp-gnutar-1.34/bin:/nix/store/0b07qazapyf3vpaikkmnirxxd1dvjb3b-gzip-1.12/bin:/nix/store/ir6kvyhfrpm20pqz3d7wxrbyrqd6ylkc-bzip2-1.0.8-bin/bin:/nix/store/xwg1p2s1mcl3qj193yw1c3sv0cpqlz5s-gnumake-4.3/bin:/nix/store/2r9n7fz1rxq088j6mi5s7izxdria6d5f-bash-5.1-p16/bin:/nix/store/jgj30km7hyvmwrlq4yqdf74dx6apqypj-patch-2.7.6/bin:/nix/store/r75z4y813x4924czbnkhmj66f4m5l0y7-xz-5.2.5-bin/bin:/nix/store/01xvwqinks2bm4y8pxw7y6cqrp36bb7s-file-5.42/bin)
makefiles/Makefile.python.mk:1098: warning: overriding recipe for target 'temp_python'
makefiles/Makefile.python.mk:909: warning: ignoring old recipe for target 'temp_python'
g++ -fPIC -std=c++17 -fwrapv -O4 -DNDEBUG -I. -Iortools/gen -Wno-deprecated -DUSE_GLOP -DUSE_BOP    -I/nix/store/2qg94y58v1jr4dw360bmpxlrs30m31ca-protobuf-3.19.4/include -I/nix/store/acs02vfbmni10xd5pms0q46q74na3syl-abseil-cpp-20210324.2/include -I/nix/store/h94622z26xj7p35zsca9qi3dvghm9wls-cbc-2.10.4/include -I/nix/store/h94622z26xj7p35zsca9qi3dvghm9wls-cbc-2.10.4/include/coin -I/nix/store/h94622z26xj7p35zsca9qi3dvghm9wls-cbc-2.10.4/include -I/nix/store/h94622z26xj7p35zsca9qi3dvghm9wls-cbc-2.10.4/include/coin -I/nix/store/h94622z26xj7p35zsca9qi3dvghm9wls-cbc-2.10.4/include -I/nix/store/h94622z26xj7p35zsca9qi3dvghm9wls-cbc-2.10.4/include/coin -DUSE_CLP -I/nix/store/h94622z26xj7p35zsca9qi3dvghm9wls-cbc-2.10.4/include -I/nix/store/h94622z26xj7p35zsca9qi3dvghm9wls-cbc-2.10.4/include/coin -I/nix/store/h94622z26xj7p35zsca9qi3dvghm9wls-cbc-2.10.4/include -I/nix/store/h94622z26xj7p35zsca9qi3dvghm9wls-cbc-2.10.4/include/coin -DUSE_CBC -DOR_TOOLS_MAJOR=9 -DOR_TOOLS_MINOR=1 \
 -c examples/cpp/network_routing_sat.cc \
 -o objs/network_routing_sat.o
examples/cpp/network_routing_sat.cc: In member function 'void operations_research::sat::NetworkRoutingData::set_name(absl::lts_20210324::string_view)':
examples/cpp/network_routing_sat.cc:134:51: error: no match for 'operator=' (operand types are 'std::string' {aka 'std::__cxx11::basic_string<char>'} and 'absl::lts_20210324::string_view')
  134 |   void set_name(absl::string_view name) { name_ = name; }
      |                                                   ^~~~
In file included from /nix/store/42izybv2fz3ps5pcg3dvcmn85pc9406y-gcc-11.3.0/include/c++/11.3.0/string:55,
                 from /nix/store/42izybv2fz3ps5pcg3dvcmn85pc9406y-gcc-11.3.0/include/c++/11.3.0/random:40,
                 from examples/cpp/network_routing_sat.cc:30:
/nix/store/42izybv2fz3ps5pcg3dvcmn85pc9406y-gcc-11.3.0/include/c++/11.3.0/bits/basic_string.h:799:8: note: candidate: 'template<class _Tp> std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::_If_sv<_Tp, std::__cxx11::basic_string<_CharT, _Traits, _Alloc>&> std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::operator=(const _Tp&) [with _Tp = _Tp; _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]'
  799 |        operator=(const _Tp& __svt)
      |        ^~~~~~~~
/nix/store/42izybv2fz3ps5pcg3dvcmn85pc9406y-gcc-11.3.0/include/c++/11.3.0/bits/basic_string.h:799:8: note:   template argument deduction/substitution failed:
In file included from /nix/store/42izybv2fz3ps5pcg3dvcmn85pc9406y-gcc-11.3.0/include/c++/11.3.0/bits/move.h:57,
                 from /nix/store/42izybv2fz3ps5pcg3dvcmn85pc9406y-gcc-11.3.0/include/c++/11.3.0/bits/atomic_base.h:38,
                 from /nix/store/42izybv2fz3ps5pcg3dvcmn85pc9406y-gcc-11.3.0/include/c++/11.3.0/atomic:41,
                 from examples/cpp/network_routing_sat.cc:28:
/nix/store/42izybv2fz3ps5pcg3dvcmn85pc9406y-gcc-11.3.0/include/c++/11.3.0/type_traits: In substitution of 'template<bool _Cond, class _Tp> using enable_if_t = typename std::enable_if::type [with bool _Cond = false; _Tp = std::__cxx11::basic_string<char>&]':
/nix/store/42izybv2fz3ps5pcg3dvcmn85pc9406y-gcc-11.3.0/include/c++/11.3.0/bits/basic_string.h:125:8:   required by substitution of 'template<class _CharT, class _Traits, class _Alloc> template<class _Tp, class _Res> using _If_sv = std::enable_if_t<std::__and_<std::is_convertible<const _Tp&, std::basic_string_view<_CharT, _Traits> >, std::__not_<std::is_convertible<const _Tp*, const std::__cxx11::basic_string<_CharT, _Traits, _Alloc>*> >, std::__not_<std::is_convertible<const _Tp&, const _CharT*> > >::value, _Res> [with _Tp = absl::lts_20210324::string_view; _Res = std::__cxx11::basic_string<char>&; _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]'
/nix/store/42izybv2fz3ps5pcg3dvcmn85pc9406y-gcc-11.3.0/include/c++/11.3.0/bits/basic_string.h:799:8:   required by substitution of 'template<class _Tp> std::__cxx11::basic_string<char>::_If_sv<_Tp, std::__cxx11::basic_string<char>&> std::__cxx11::basic_string<char>::operator=<_Tp>(const _Tp&) [with _Tp = absl::lts_20210324::string_view]'
examples/cpp/network_routing_sat.cc:134:51:   required from here
/nix/store/42izybv2fz3ps5pcg3dvcmn85pc9406y-gcc-11.3.0/include/c++/11.3.0/type_traits:2585:11: error: no type named 'type' in 'struct std::enable_if<false, std::__cxx11::basic_string<char>&>'
 2585 |     using enable_if_t = typename enable_if<_Cond, _Tp>::type;
      |           ^~~~~~~~~~~
In file included from /nix/store/42izybv2fz3ps5pcg3dvcmn85pc9406y-gcc-11.3.0/include/c++/11.3.0/string:55,
                 from /nix/store/42izybv2fz3ps5pcg3dvcmn85pc9406y-gcc-11.3.0/include/c++/11.3.0/random:40,
                 from examples/cpp/network_routing_sat.cc:30:
/nix/store/42izybv2fz3ps5pcg3dvcmn85pc9406y-gcc-11.3.0/include/c++/11.3.0/bits/basic_string.h:679:7: note: candidate: 'std::__cxx11::basic_string<_CharT, _Traits, _Alloc>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::operator=(const std::__cxx11::basic_string<_CharT, _Traits, _Alloc>&) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]'
  679 |       operator=(const basic_string& __str)
      |       ^~~~~~~~
/nix/store/42izybv2fz3ps5pcg3dvcmn85pc9406y-gcc-11.3.0/include/c++/11.3.0/bits/basic_string.h:679:37: note:   no known conversion for argument 1 from 'absl::lts_20210324::string_view' to 'const std::__cxx11::basic_string<char>&'
  679 |       operator=(const basic_string& __str)
      |                 ~~~~~~~~~~~~~~~~~~~~^~~~~
/nix/store/42izybv2fz3ps5pcg3dvcmn85pc9406y-gcc-11.3.0/include/c++/11.3.0/bits/basic_string.h:689:7: note: candidate: 'std::__cxx11::basic_string<_CharT, _Traits, _Alloc>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::operator=(const _CharT*) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]'
  689 |       operator=(const _CharT* __s)
      |       ^~~~~~~~
/nix/store/42izybv2fz3ps5pcg3dvcmn85pc9406y-gcc-11.3.0/include/c++/11.3.0/bits/basic_string.h:689:31: note:   no known conversion for argument 1 from 'absl::lts_20210324::string_view' to 'const char*'
  689 |       operator=(const _CharT* __s)
      |                 ~~~~~~~~~~~~~~^~~
/nix/store/42izybv2fz3ps5pcg3dvcmn85pc9406y-gcc-11.3.0/include/c++/11.3.0/bits/basic_string.h:700:7: note: candidate: 'std::__cxx11::basic_string<_CharT, _Traits, _Alloc>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::operator=(_CharT) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]'
  700 |       operator=(_CharT __c)
      |       ^~~~~~~~
/nix/store/42izybv2fz3ps5pcg3dvcmn85pc9406y-gcc-11.3.0/include/c++/11.3.0/bits/basic_string.h:700:24: note:   no known conversion for argument 1 from 'absl::lts_20210324::string_view' to 'char'
  700 |       operator=(_CharT __c)
      |                 ~~~~~~~^~~
/nix/store/42izybv2fz3ps5pcg3dvcmn85pc9406y-gcc-11.3.0/include/c++/11.3.0/bits/basic_string.h:717:7: note: candidate: 'std::__cxx11::basic_string<_CharT, _Traits, _Alloc>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::operator=(std::__cxx11::basic_string<_CharT, _Traits, _Alloc>&&) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]'
  717 |       operator=(basic_string&& __str)
      |       ^~~~~~~~
/nix/store/42izybv2fz3ps5pcg3dvcmn85pc9406y-gcc-11.3.0/include/c++/11.3.0/bits/basic_string.h:717:32: note:   no known conversion for argument 1 from 'absl::lts_20210324::string_view' to 'std::__cxx11::basic_string<char>&&'
  717 |       operator=(basic_string&& __str)
      |                 ~~~~~~~~~~~~~~~^~~~~
/nix/store/42izybv2fz3ps5pcg3dvcmn85pc9406y-gcc-11.3.0/include/c++/11.3.0/bits/basic_string.h:785:7: note: candidate: 'std::__cxx11::basic_string<_CharT, _Traits, _Alloc>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::operator=(std::initializer_list<_Tp>) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]'
  785 |       operator=(initializer_list<_CharT> __l)
      |       ^~~~~~~~
/nix/store/42izybv2fz3ps5pcg3dvcmn85pc9406y-gcc-11.3.0/include/c++/11.3.0/bits/basic_string.h:785:42: note:   no known conversion for argument 1 from 'absl::lts_20210324::string_view' to 'std::initializer_list<char>'
  785 |       operator=(initializer_list<_CharT> __l)
      |                 ~~~~~~~~~~~~~~~~~~~~~~~~~^~~
make[1]: *** [makefiles/Makefile.cpp.mk:320: objs/network_routing_sat.o] Error 1
make[1]: Leaving directory '/build/source'
make: *** [makefiles/Makefile.cpp.mk:537: test_cc_cpp] Error 2

tobim · 2022-08-14T21:52:44Z

Part of the reason cxxStandard was introduced as an overridable option was to avoid breaking or-tools (#143967), and indeed, this PR breaks or-tools.

Apparently absl::string_view doesn't implement a conversion operator to std::string_view, which would be needed to assign to std::string. I pushed a patch for or-tools to fix that failure.

I think we can try to switch over to the std provided implementations with 202206 and start phasing out the older LTSs soon.

tobim · 2022-08-17T18:54:12Z

@andersk would be more open to accepting this if I added myself as co-,maintainer?

andersk · 2022-08-18T21:38:00Z

Apparently absl::string_view doesn't implement a conversion operator to std::string_view, which would be needed to assign to std::string. I pushed a patch for or-tools to fix that failure.

I mean, of course it doesn’t, because you’ve now configured it to never use std::string_view. Since that isn’t a configuration or-tools supports, I’m somewhat worried that we’ll need to maintain a growing number of patches like this over time.

What do you think of allowing cxxStandard to vary as long as static = true (which it is for or-tools)?

I think we can try to switch over to the std provided implementations with 202206 and start phasing out the older LTSs soon.

Does 202206 change something relevant here?

Abseil’s options.h documents that forcing the std implementations “requires that all code using Abseil is built in C++17 mode or later”, which was evidently not the case at least as of #143967.

tobim · 2022-08-19T04:15:48Z

I mean, of course it doesn’t, because you’ve now configured it to never use std::string_view. Since that isn’t a configuration or-tools supports, I’m somewhat worried that we’ll need to maintain a growing number of patches like this over time.

What do you think of allowing cxxStandard to vary as long as static = true (which it is for or-tools)?

I think it's better to patch or-tools than to complicate the interface to abseil. I wouldn't expect this to require too many patches like this, but the risk of it is the reason I suggested to switch to the standard implementations in a somewhat timely manner.

I think we can try to switch over to the std provided implementations with 202206 and start phasing out the older LTSs soon.

Does 202206 change something relevant here?

No, but in my opinion a new version is a good opportunity to do the switch.

Abseil’s options.h documents that forcing the std implementations “requires that all code using Abseil is built in C++17 mode or later”, which was evidently not the case at least as of #143967.

grpc and dependents work fine with C++17. Fedora, Arch, and Homebrew all do this already, so we wouldn't be the only ones.

ilya-fedin · 2022-08-19T08:53:02Z

Personally I like the ability to choose cxxStandard of abseil. This allows to build with recommended settings of upstreams.

tobim · 2022-09-17T20:03:36Z

What do you think of allowing cxxStandard to vary as long as static = true (which it is for or-tools)?

@andersk does this offer still stand? I'd like to get the ball rolling again.

tobim · 2022-10-18T21:51:50Z

Sorry about the mass-notification. I made a mistake while re-targeting.

dasJ · 2022-10-18T21:54:37Z

Please open a new pull request as people who were requested will now get a notification for any action

tobim requested review from FRidh and jonringer as code owners August 14, 2022 05:34

github-actions bot added the 6.topic: python Python is a high-level, general-purpose programming language. label Aug 14, 2022

tobim force-pushed the pkgs/abseil-cpp/lock-vocabulary-type-impls branch from 41a9163 to 162b058 Compare August 14, 2022 05:44

ofborg bot requested review from andersk, ilya-fedin and oxalica August 14, 2022 05:53

ofborg bot added 10.rebuild-darwin: 101-500 This PR causes between 101 and 500 packages to rebuild on Darwin. 10.rebuild-linux: 101-500 This PR causes between 101 and 500 packages to rebuild on Linux. labels Aug 14, 2022

tobim force-pushed the pkgs/abseil-cpp/lock-vocabulary-type-impls branch from 162b058 to e0ea214 Compare August 14, 2022 06:15

ilya-fedin reviewed Aug 14, 2022

View reviewed changes

pkgs/applications/networking/instant-messengers/telegram/tdesktop/default.nix Outdated Show resolved Hide resolved

ilya-fedin reviewed Aug 14, 2022

View reviewed changes

pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved

ofborg bot requested a review from ilya-fedin August 14, 2022 07:41

andersk suggested changes Aug 14, 2022

View reviewed changes

ofborg bot requested a review from andersk August 14, 2022 21:53

SuperSandro2000 added the 2.status: merge conflict This PR has merge conflicts with the target branch label Oct 3, 2022

tobim force-pushed the pkgs/abseil-cpp/lock-vocabulary-type-impls branch from 93b189b to cb663f9 Compare October 18, 2022 17:25

ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Oct 18, 2022

ofborg bot requested a review from NickCao October 18, 2022 17:36

tobim requested review from a team, Ericson2314, Profpatsch, adisbladis, cdepillabout, dasJ, edolstra, infinisil, layus, maralorn, nbp, sternenseemann and zakame as code owners October 18, 2022 21:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

abseil-cpp: lock in provided vocabulary types#186490

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

tobim commented Aug 13, 2022

Uh oh!

SuperSandro2000 commented Aug 14, 2022

Uh oh!

tobim commented Aug 14, 2022

Uh oh!

Uh oh!

Uh oh!

andersk left a comment

Uh oh!

tobim commented Aug 14, 2022

Uh oh!

tobim commented Aug 17, 2022

Uh oh!

andersk commented Aug 18, 2022

Uh oh!

tobim commented Aug 19, 2022

Uh oh!

ilya-fedin commented Aug 19, 2022

Uh oh!

tobim commented Sep 17, 2022

Uh oh!

tobim commented Oct 18, 2022

Uh oh!

dasJ commented Oct 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

tobim commented Aug 13, 2022

Description of changes

Things done

Uh oh!

SuperSandro2000 commented Aug 14, 2022

Uh oh!

tobim commented Aug 14, 2022

Uh oh!

Uh oh!

Uh oh!

andersk left a comment

Choose a reason for hiding this comment

Uh oh!

tobim commented Aug 14, 2022

Uh oh!

tobim commented Aug 17, 2022

Uh oh!

andersk commented Aug 18, 2022

Uh oh!

tobim commented Aug 19, 2022

Uh oh!

ilya-fedin commented Aug 19, 2022

Uh oh!

tobim commented Sep 17, 2022

Uh oh!

tobim commented Oct 18, 2022

Uh oh!

dasJ commented Oct 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants