-
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
Closed
Closed
[bde] Updated to 3.117.0 #31644
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
6cd8230
Update bde port
osubboo f8a6675
Update version in bde.json
adamncasey 3f357f8
Remove ci.baseline.txt skip.
BillyONeal 742d743
Fix version database.
BillyONeal 5a9ac9c
fix port-version
BillyONeal 42c9919
Stop building for android
adamncasey 6ff3dc4
Update version
adamncasey File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,21 @@ | ||
| { | ||
| "name": "bde", | ||
| "version": "3.2.0.0", | ||
| "port-version": 5, | ||
| "version": "3.117.0.0", | ||
| "description": "Basic Development Environment - a set of foundational C++ libraries used at Bloomberg.", | ||
| "supports": "!windows & !arm" | ||
| "supports": "!windows & !arm & !android", | ||
| "dependencies": [ | ||
| { | ||
| "name": "vcpkg-cmake", | ||
| "host": true | ||
| }, | ||
| { | ||
| "name": "vcpkg-cmake-config", | ||
| "host": true | ||
| } | ||
| ], | ||
| "features": { | ||
| "cxx17": { | ||
| "description": "Enable compiler C++17." | ||
| } | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?
See https://learn.microsoft.com/en-us/vcpkg/contributing/maintainer-guide#do-not-use-features-to-implement-alternatives
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.
Abseil does not follow such a pattern though, because they namespace all their polyfills. That is, customer code can be written always naming
absl::stuffand it will work regardless of this setting.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.
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:
https://learn.microsoft.com/en-us/vcpkg/contributing/maintainer-guide#a-feature-may-replace-polyfills-with-aliases-provided-that-replacement-is-baked-into-the-installed-tree
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.