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

Fixing std::optional handling being redundant in Boost version > 1.80 #1721

Merged
merged 1 commit into from
Feb 27, 2024

Conversation

talregev
Copy link
Contributor

In macos vm, recently we get a newer version of boost (1.84).
They already implemented in the new version the function that gtsam implemented.
So I am not sure the correct version of boost that it begin to happen, so I put version 1.80.
We can tune the version later (when we have another issue similar to that in different boost version).

@varunagrawal @ProfFan
Thank you for your time and effort for review this PR.

@varunagrawal
Copy link
Collaborator

We're looking to remove boost altogether so we probably don't need this PR

@varunagrawal
Copy link
Collaborator

Also the PR comment doesn't mention what the issue is or what it fixes.

@talregev
Copy link
Contributor Author

Also the PR comment doesn't mention what the issue is or what it fixes.

I can fix the comment. That is for code review.

I hope you will fix the boost issue soon!.

@ProfFan
Copy link
Collaborator

ProfFan commented Feb 11, 2024

So this is for std::optional?

@ProfFan
Copy link
Collaborator

ProfFan commented Feb 24, 2024

@talregev Could you edit the title to say the exact issue this fixes? Thank you very much!

@ProfFan ProfFan reopened this Feb 24, 2024
@ProfFan
Copy link
Collaborator

ProfFan commented Feb 24, 2024

Also is this change in boost happening in 1.80 only? Could you link the Boost change note as well?

@talregev talregev changed the title Bug fix for macos: Support new version of boost along side old version. Bug fix for CI macos: Support new version of boost along side old version. Feb 24, 2024
@talregev
Copy link
Contributor Author

talregev commented Feb 24, 2024

I don't mind changing the title. I added CI there. You can write me the title that it will be more informative for you.

@ProfFan Thank you for opening this PR again.

@talregev
Copy link
Contributor Author

Also is this change in boost happening in 1.80 only? Could you link the Boost change note as well?

As I stated in the first msg, I am not sure what version this was happening.
My guess is it started at 1.8 and we can tune this later.

@ProfFan
Copy link
Collaborator

ProfFan commented Feb 24, 2024

Also is this change in boost happening in 1.80 only? Could you link the Boost change note as well?

As I stated in the first msg, I am not sure what version this was happening.
My guess is it started at 1.8 and we can tune this later.

Please check the Boost release notes. Title could be 'Fixing std::optional handling being redundant in Boost version > 1.xx'

But be sure to check the version number, we could accidentally break others' code!

@talregev
Copy link
Contributor Author

talregev commented Feb 25, 2024

Also is this change in boost happening in 1.80 only? Could you link the Boost change note as well?

As I stated in the first msg, I am not sure what version this was happening.
My guess is it started at 1.8 and we can tune this later.

Please check the Boost release notes. Title could be 'Fixing std::optional handling being redundant in Boost version > 1.xx'

But be sure to check the version number, we could accidentally break others' code!

Can you help me and search also?
I think it found in 1.80
https://www.boost.org/users/history/version_1_80_0.html

Optional:
Added specializations for std::hash<boost::optional>. This is a breaking change for programs that define such specializations themselves. For more details see specs.

Copy link
Collaborator

@ProfFan ProfFan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good, please change the title and add a comment above the code that says // See CHANGELOG: <link>

@talregev
Copy link
Contributor Author

talregev commented Feb 25, 2024

Good, please change the title and add a comment above the code that says // See CHANGELOG: <link>

Can you write the exact title you want?

@ProfFan ProfFan changed the title Bug fix for CI macos: Support new version of boost along side old version. Fixing std::optional handling being redundant in Boost version > 1.80 Feb 26, 2024
@@ -11,6 +11,8 @@

// Defined only if boost serialization is enabled
#ifdef GTSAM_ENABLE_BOOST_SERIALIZATION
// Only for old boost
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Only for old boost
// NOTE: Added in Boost 1.80
// See Boost CHANGELOG: https://www.boost.org/users/history/version_1_80_0.html

@varunagrawal
Copy link
Collaborator

I don't think this is the cause of the issue. I looked at the commit history of boost/serialization/optional.hpp and this commit boostorg/serialization@ff0a5f9 was quite enlightening. Note that there have been only 2 changes in the file since 2020.

Essentially, the authors fixed the inclusion of std::optional primarily for upgrading to C++17. There have been related updates to add save, load, etc, to accommodate this. The easier fix is to check for the C++ version and conditionally compile based on that. I have included the fix in #1723.

@varunagrawal
Copy link
Collaborator

I looked into it more closely, and the real issue is this PR: https://github.com/boostorg/serialization/pull/163/files

It got merged on 09/02/2023 and std_optional_serialization.hpp is based on the changes in that PR. Thus we are seeing duplicates just for 1.84.0.

@ProfFan
Copy link
Collaborator

ProfFan commented Feb 27, 2024

Since I got no response from @talregev I'll just merge this PR so the issue gets fixed fast and we don't waste any more cycles on this. @varunagrawal I believe the actual commit is boostorg/serialization@61a2b12, the PR you linked was reverted. But you are right, correct boost version is 1.84 due to the addition of std::optional serialization. Could you rebase on develop and make the necessary corrections? Thank you very much!

@ProfFan ProfFan merged commit 5b96684 into borglab:develop Feb 27, 2024
62 checks passed
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