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

replace boost::optional::has_value with more compatible is_initialized #3219

Merged
merged 1 commit into from
Jul 19, 2021

Conversation

nilsnolde
Copy link
Member

@nilsnolde nilsnolde commented Jul 19, 2021

also discovered during building with boost 1.62. has_value() was introduce in 1.68: https://www.boost.org/doc/libs/1_69_0/libs/optional/doc/html/boost_optional/relnotes.html#boost_optional.relnotes.boost_release_1_68

has_value() is just an alias for is_initialized() which has more compatibility: boostorg/optional@5182f7f#diff-39c347130088a3259b9ba81c9aa6f72ed620fb23ff0cc52484c6088dc8ed6661R1336

@nilsnolde
Copy link
Member Author

btw, I know it's slightly annoying to have those backwards compat PRs for boost here and there, but (for me at least) less annoying than building boost myself in the case where we'd upgrade the min version. My plan is to build the python packages at least once a month, so I can guarantee we'll be compatible down to 1.62 for the time being.

@merkispavel
Copy link
Contributor

merkispavel commented Jul 19, 2021

btw, I know it's slightly annoying to have those backwards compat PRs for boost here and there, but (for me at least) less annoying than building boost myself

ah, it's second when i forget about boost compat. Sorry for that and thanks for the fix!
related #3035

@nilsnolde nilsnolde merged commit 2b13b5b into master Jul 19, 2021
@kevinkreiser
Copy link
Member

we can make this easier on ourselves by having a base docker image with boost built from source to the lowest version we support. we used to cheat by setting the version to the one that came with the ubuntu release that we were targetting but we've since moved to 20.04 and of course that has newer boost (also in ci) than the previous target. it sounds like at least @nilsnolde is targetting an older version of boost on some platforms which means we cant just up the boost version to the one that comes with 20.04.

@nilsnolde nilsnolde deleted the nn-fix-boost-optional-compat branch July 19, 2021 17:59
@nilsnolde
Copy link
Member Author

how low you'd like to go with boost? 1.51 seems a little radical ;) (if it's even possible without quite some changes..).

anyone here using this project who relies on boost < v1.62?

@kevinkreiser
Copy link
Member

i guess i was advocating for adhering to what cmake says is required, which is boost 1.51, which as i was aluding to is probably from 16.04 or maybe even 14.04 (trusty?). its possible that that doesnt even work at this point though so maybe whatever 18.04 has?

@nilsnolde
Copy link
Member Author

18.04. is on 1.65: https://pkgs.org/search/?q=Boost

Would you be ok with 1.62 as min version? I can amend the docker file with a custom boost. Not now, but at some point :)

@kevinkreiser
Copy link
Member

@nilsnolde let me start an issue about this and ill put my thoughts there

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants