-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
[bde] Updated to 3.117.0 #31644
[bde] Updated to 3.117.0 #31644
Conversation
Disable windows and arm, at least for now It'd be great to enable arm64-osx (macos?) soon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed version database for you :)
I removed the ci.baseline.txt skips for you so that bde is actually tested; this may result in failures because it was previously untested on Linux. If you don't think it should support android
, & !android
should get added to supports
.
Thanks for your contribution to vcpkg!
} | ||
], | ||
"features": { | ||
"cxx17": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like an 'alternative'; is it possible for downstream customers to be agnostic about this setting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a good question. I think the idea was to follow the abseil pattern here (maybe they're following something else which came before it).
There's definite trickiness around this setting and consuming the library - the header files of this library generally need to be built with a c++ standard identical to the static library linked against. I'm not sure what alternatives exist here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a good question. I think the idea was to follow the abseil pattern here (maybe they're following something else which came before it).
Abseil does not follow such a pattern though, because they namespace all their polyfills. That is, customer code can be written always naming absl::stuff
and it will work regardless of this setting.
There's definite trickiness around this setting and consuming the library - the header files of this library generally need to be built with a c++ standard identical to the static library linked against. I'm not sure what alternatives exist here
That's also something abseil doesn't do; there is a feature there but it bakes the 'polyfills or not' setting directly into the bits so that downstream C++ version need not agree.
In general we do not expect a given program to only ever be built with a consistent C++ version.
I think the correct think to do here is thus to turn on the setting and say "vcpkg's bde says you are using c++17+; if you want something else you must use overlays".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation - and time taken so far to consider this PR! 🙂. I might be misunderstanding your description since I haven't used abseil, but I think bde is in a similar situation wrt to polyfills - everything is namespaced.
In general we do not expect a given program to only ever be built with a consistent C++ version.
Makes sense, although as I'm sure you've realised - bde does expect that ~all usages of it's header files are linked against a version of the library which has been built with the same C++ standard version.
I think your suggestion is workable but could make it difficult to change the standard in future. I'll need to discuss internally to figure out what's feasible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the exception we carved out specifically for what abseil was doing:
In particular, the requirement to bake the feature answer into the resulting installed tree. bde[core]
needs to use the polyfills even if downstream someone uses a C++17 compiler.
Unfortunately the C++ ecosystem writ large does not choose a consistent language version at any given time, so we require things indexed in our curated registry to deal with that situation.
I don't specifically know why this wouldn't work, but let's fix that another time
Please sign the CLA. |
Note: I will be converting your PR to draft status. When you respond, please revert to "ready for review". |
Closing this in favour of a PR opened from the bloomberg fork of vcpkg, with the intention of satisfying the CLA bot. |
Fixes #18937
The "supports" clause reflects platforms that may be fixed by this new versionAny fixed CI baseline entries are removed from that file../vcpkg x-add-version --all
and committing the result.