Skip to content

[abseil] Configure abseil to use std:: types when feature cxx17 is enabled#11039

Merged
strega-nil merged 1 commit intomicrosoft:masterfrom
cneumann:abseil-cxx17-use-stdlib-types
May 1, 2020
Merged

[abseil] Configure abseil to use std:: types when feature cxx17 is enabled#11039
strega-nil merged 1 commit intomicrosoft:masterfrom
cneumann:abseil-cxx17-use-stdlib-types

Conversation

@cneumann
Copy link
Contributor

Patches absl/base/options.h type selection macros to use either value
0 (when feature cxx17 is not enabled) to ensure that always absl:: types are used. This was already
done by the pre-existing fix-lnk2019-error.patch in the port.
When feature cxx17 is enabled instead value 1 is used to force abseil to use
the std:: types.

Abseil can use std::any, std::optional, std::string_view, and std::variant
or its own internal replacement types. Which ones are used
is configured by setting macros in absl/base/options.h to values 0/1/2
where 0 means always use absl:: types, 1 is always use std:: types (and
require a C++17 library), 2 detect if the standard library supports the
types and use them if available (see also comments in absl/base/options.h).

@LilyWangL LilyWangL self-requested a review April 27, 2020 01:54
@LilyWangL LilyWangL self-assigned this Apr 27, 2020
@LilyWangL
Copy link
Contributor

Thanks for your PR! Can you please correct the Version field in the CONTROL file? You can get more information from maintainer-guide.md.

@LilyWangL LilyWangL changed the title Configure abseil to use std:: types when feature cxx17 is enabled [abseil] Configure abseil to use std:: types when feature cxx17 is enabled Apr 28, 2020
Adds a patch changing macros in absl/base/options.h to
always use the std:: namespace types instead of the
absl:: namespace replacements (for any, optional,
string_view, variant).
The upstream version of options.h uses compiler/library
feature detection to decide this, but patch
fix-lnk2019-error.patch hard codes use of absl:: types,
thus rendering setting CMAKE_CXX_STANDARD to 17 in the
port file ineffective. Since auto detection is problematic
from an ABI point of view (see comments in
absl/base/options.h for details), this applies an
alternate patch for fix-lnk2019-error.patch when feature
cxx17 is enabled.
@cneumann
Copy link
Contributor Author

Ah, sorry for missing that. I've updated the version and also updated the commit message to include the port tag and hopefully be a bit clearer about what is being fixed and how.

@LilyWangL
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@cneumann
Copy link
Contributor Author

Hmm, I'm having trouble understanding the cause for the failures reported above. For example looking at the details of vcpkg-osx-PR-test it looks like the port that fails is mlpack, but as far as I can tell abseil is not a direct nor indirect dependency of mlpack - how did my change manage to break it? Any pointers are appreciated :)

@LilyWangL
Copy link
Contributor

LilyWangL commented Apr 29, 2020

Hmm, I'm having trouble understanding the cause for the failures reported above. For example looking at the details of vcpkg-osx-PR-test it looks like the port that fails is mlpack, but as far as I can tell abseil is not a direct nor indirect dependency of mlpack - how did my change manage to break it? Any pointers are appreciated :)

It's not related with this PR, this error will be fixed in PR #11063.

@LilyWangL
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@LilyWangL LilyWangL added the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label Apr 30, 2020
@strega-nil
Copy link
Contributor

This is great, thanks @cneumann :)

@strega-nil strega-nil merged commit 41f360b into microsoft:master May 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants