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

fix building with boost 1.75 #8606

Merged
merged 2 commits into from Dec 16, 2020
Merged

Conversation

JanMarvin
Copy link
Contributor

Intent

This pull requests solves building rstudio with boost 1.75 which recently dropped in Arch Linux.

Approach

The required header files were included as well as the required namespace.

QA Notes

This builds with my rstudio PKGBUILD (this PKGBUILD includes another boost patch, though I am not sure what that was fixing [lost track of rstudios development a while ago]).

I expect this to be backward compatible if the included header files exist in previous boost releases, but I have not tested this. I have also not tested this with the included rstudio boost.

I have signed a contributor agreement a long time ago.

@kevinushey
Copy link
Contributor

kevinushey commented Dec 14, 2020

Thanks for the PR!

IIUC we were previously getting away with this via this in CMake:

# global directives
add_definitions(-DBOOST_ENABLE_ASSERT_HANDLER)
add_definitions(-DBOOST_BIND_GLOBAL_PLACEHOLDERS)

Is that effectively removed from Boost 1.75? If so, we might want to remove that line as well and ensure we're properly using Boost's placeholders in a forward-compatible way.

@JanMarvin
Copy link
Contributor Author

Thanks for the hint (I didn't know the reason boost for the behavior change). Presumably including placeholder is not required and using the macro is no longer possible according to this commit:

boostorg/bind@2797f0d

@gtritchie gtritchie added the tech debt internal tech debt with no direct benefit to customer. label Dec 14, 2020
@JanMarvin
Copy link
Contributor Author

JanMarvin commented Dec 16, 2020

That's a hella lot of files I touched now. Did not build it, is that what you wanted @kevinushey ?
edit: Should be boost/bind/bind.cpp

@kevinushey
Copy link
Contributor

Oof, thanks! I didn't expect it would've required changes in so many source files, but I guess that ultimately makes sense.

We're currently in the process of wrapping up a 1.4 release so I don't think we can merge this immediately, but will plan to do so as part of the next 1.4 patch release (which should be released relatively soon after the initial 1.4 release).

@JanMarvin
Copy link
Contributor Author

Happy to help. Just to clarify, my first commit actually fixes a build error I was seeing, the second commit merely fixes warning messages. Good luck with the release and stay healthy

@jmcphers
Copy link
Member

@JanMarvin I pulled your branch and built it locally w/o issue. I'm going to rebase your PR onto our current patch release branch, v1.4-juliet-rose, and merge it there. That branch will be merged to master shortly after RStudio 1.4 is released.

@jmcphers jmcphers changed the base branch from master to v1.4-juliet-rose December 16, 2020 22:58
@jmcphers jmcphers merged commit c8b78eb into rstudio:v1.4-juliet-rose Dec 16, 2020
@jmcphers
Copy link
Member

@JanMarvin Thanks very much for the contribution!

@Enchufa2
Copy link
Contributor

Did you build this against Boost 1.75? I've incorporated the patch in Fedora rawhide, which contains Boost 1.75 now, and compilation fails. I think we still need -DBOOST_BIND_GLOBAL_PLACEHOLDERS, which was removed in this PR.

@JanMarvin
Copy link
Contributor Author

yes, I build current master with boost 1.75

@Enchufa2
Copy link
Contributor

I see the following applying this patch to current release (1.4.1103):

/builddir/build/BUILD/rstudio-1.4.1103/src/cpp/session/SessionConsoleProcessSocketTests.cpp: In member function 'bool rstudio::session::console_process::{anonymous}::SocketClient::connectToServer()':
/builddir/build/BUILD/rstudio-1.4.1103/src/cpp/session/SessionConsoleProcessSocketTests.cpp:249:55: error: '::_1' has not been declared
  249 |                                           &client_, ::_1, ::_2));

Any idea why?

@JanMarvin
Copy link
Contributor Author

maybe someone introduced a new boost function not covered by this pull request? But I did build yesterdays master, which contains this patch.

@Enchufa2
Copy link
Contributor

Nope, that was there in your PR: https://github.com/JanMarvin/rstudio/blob/a73589c69c62b55935c2b0936bbe7ffe52fe0a49/src/cpp/session/SessionConsoleProcessSocketTests.cpp#L249. For some reason, ::_1 does not found the placeholder in the global namespace. I'm rebuilding with _1 instead, which should use the websocketpp placeholder. Let's see.

@JanMarvin
Copy link
Contributor Author

If you could share a link to the PKGBUILD or how it's called in Fedora, I could have a look. Right now I'd say that applying the pull request somehow did not work as expected. My build files are here if you want to have a look.

@Enchufa2
Copy link
Contributor

Using _1, _2 instead of ::_1, ::_2 and removing using websocketpp::lib::placeholders::_1; and using websocketpp::lib::placeholders::_2; did the trick. It could be the compiler version, the flags... I don't know. But anyway, it makes no sense to declare websocketpp's placeholders and then avoid them with :: to use boost ones. I'll open another PR for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tech debt internal tech debt with no direct benefit to customer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants