Boost Conan 2.0 compatibility #16222
Closed
Conversation
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
|
🤖 Beep Boop! This pull request is making changes to 'recipes/boost//'. 👋 @Hopobcn @jwillikers you might be interested. 😉 |
This comment has been minimized.
This comment has been minimized.
ghost
mentioned this pull request
|
I detected other pull requests that are modifying boost/all recipe:
This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there. |
ghost
mentioned this pull request
3 tasks
I don't know how you are using boost recipe, but |
|
Hi @System-Arch - the boost recipe as it is on master has passed our validation with Conan 2.0 and we are able to generate packages for it with Conan 2.0 at least for the platforms that we are currently testing. Would you be able to provide more details about what errors you are getting, and how we can reproduce them, so that we can have a look at this? Thanks! |
Hi @jcar87, Sure, I am taking advantage of the clean orthogonality of commands in Conan 2.0 and am issuing the following discrete commands:
While the build succeeds with the CCI recipe in master, the export-pkg stage does virtually nothing (because it can't find the built artifacts):
which leads to the following test failure:
With the changes in the proposed PR, this workflow works fine because it honors the fact that the cache package_folder is not computed until the package method is called. Please let me know if you'd like more details. Thanks |
|
It doesn't seem to be a neat fix. We should still rely on install feature of b2 somehow, instead of manually copying binaries. The problem of current boost recipe is that build & installation occur in I don't know how to achieve that with b2, there is no DESTDIR. I've seen some discussions here: boostorg/boost_install#14 |
SpaceIm
reviewed
Feb 22, 2023
| @@ -59,4 +59,4 @@ def test(self): | |||
| if not can_run(self): | |||
| return | |||
| with chdir(self, self.folders.build_folder): | |||
| self.run(f"ctest --output-on-failure -C {self.settings.build_type}", env="conanrun") | |||
| self.run(f"ctest --output-on-failure -C {self.settings.build_type}") | |||
There was a problem hiding this comment.
env="conanrun" must not be removed
There was a problem hiding this comment.
env="conanrun" must not be removed
Hi @SpaceIm,
Please see the conversation with @memsharded (conan-io/conan#13138) where he explains why it should be removed when using Conan to manage the toolchain. In particular, if one is using a host profile to specify the GCC version via a [tool_requires]. we need both the runtime and buildtime environments for dynamically-linked tests to find the G++ runtine (libstdc++.so).
There was a problem hiding this comment.
I see, but I think test() @memsharded is referring on (CMake.test()?) is totally different than test() of test package and self.run() command we are running.
So I don't say there is not also an issue here when someone has cmake in tool_requires, but by default a raw self.run is executed in build scope, so I don't see how it couldn't lead to a regression. What you want is to inject env vars of cmake into run scope, or inject both build & run scope into this self.run
There was a problem hiding this comment.
Hi @spacelm,
I agree the circumstances in the thread with @memsharded were slightly different, but they are, in fact, a subset of what we have here. In both instances, the test_package test() method is calling ctest, so we need to be able to find that, which may be provided as part of a Conan-managed toolchain via [tool_requires]. The second issue, and it perhaps suggests that we really do need a [test_requires] feature in the host profile, is that the tests that use dynamic linking need to be able to see the runtime shared libraries provided by the compiler (GCC in this case) but those appear to not be part of the runtime environment when GCC is a [tool_requires]. I will add this new use case to the Conan issue thread to see what insights @memsharded can provide. Thanks.
There was a problem hiding this comment.
The removal of the env altogether will cause the tests to fail on Windows when shared=True, CI will fail in this instance:
The following tests FAILED:
1 - boost_random (Exit code 0xc0000135
)
3 - boost_test (Exit code 0xc0000135
)
4 - coroutine_test (Exit code 0xc0000135
)
5 - chrono_test (Exit code 0xc0000135
)
6 - boost_fiber (Exit code 0xc0000135
)
7 - boost_json (Exit code 0xc0000135
)
8 - boost_nowide (Exit code 0xc0000135
)
9 - boost_locale (Exit code 0xc0000135
where the exit code matches STATUS_DLL_NOT_FOUND
Based on what you are describing, I believe the changes introduced in #16348 should handle the case where you are expecting the gcc runtime libraries from a tool_requires. with an interest of moving forward with this PR, I'll restore this to how it is on master currently, and if there are still issues running tests in this instance, we han investigate separately - as it's something that is not specific to the boost test_package.
Hi @SpaceIm, Thanks for bringing the related Boost discussion to our attention. It seems to suggest that manual copying is, in fact, the way that other systems typically handle this situation. Quoting from the thread:
Thus, it seems that, while the copying approach implemented in this PR may not be the most ideal solution, it may be the best one can do given the inherent limitations in b2 and the Boost build infrastructure. Thanks |
|
@grafikrobot can you comment on this issue? for me personally, the removal of |
@SSE4 I don't know what you mean by that. B2 has had support for I'm failing to follow what this change is about, and the comments on it though :-) |
Hi @grafikrobot, The issue this PR attempts to address is the ability to use the Conan 2.0 modular package creation process where source, build, package, and update can be handled as discrete steps. When operating in this manner Conan doesn't know the ultimate location of the built artifacts in the Conan cache at build time--only at package time. Strategies like DESTDIR work well with this separation of concerns because the value of DESTDIR does not need to be known or specified until invoking the install target of the package Makefile. The current Conan boost recipe results in an incomplete (virtually empty) package when invoked in the module fashion described above because the b2 install puts the resulting bits in the "wrong" place. The changes proposed in this PR remedy this issue by letting b2 build boost such that the resulting artifacts are placed in the well-known "stage" area. The Conan package() method that traditionally installs these bits in the Conan cache now does so by copying them from the "stage" area to the now-known cache area. Let us know if this helps clarify things and/or if you see a way to do the same thing using b2 where build and install are discrete operations. Thanks |
|
@System-Arch thanks for the explanation.
After reading the thread, changes, and the current Boost recipe my conclusion is.. If we want to fix this properly one should just do the the build part of B2 in the HTH |
This comment has been minimized.
This comment has been minimized.
Hi @grafikrobot, Thank you for the additional insights. I have revised the package() method of this PR to call "b2 install" in place of the explicit copy commands as you suggested. I also tried removing the "stage" option but that appears to make no difference as the build concludes with a message that the libraries can be found in src/stage/lib. The older bjam documentation seems to confirm this by stating that not passing any target is the same passing "stage" (see https://theboostcpplibraries.com/introduction-installation). Thus I left it in for clarity. Please let me know if you agree with this assessment or whether I am missing something more subtle. Thanks. |
Well that's unfortunate. I don't remember it being that way int he past. But there's been a lot of changes to account for the cmake install support. I'll see about making just building work on the Boost side. Since Ci checked the build code and indeed there is no default only-build target. Regardless I think this change looks good now. |
grafikrobot
previously approved these changes
Feb 25, 2023
This comment has been minimized.
This comment has been minimized.
Hi @grafikrobot, Thanks for your review. Unfortunately, I discovered one more "fly in the ointment"--namely that building with the "headers_only" option was no longer working. In searching the b2 doc and on-line discussions, it seems that b2 doesn't have support for this paradigm. Thus I added explicit copying for this use case. So, if you are looking into enhancing the Boost build process, it would be nice to have the ability to specify a headers-only option at both build and install times. Also, helpful would be the ability to specify "install" without having to repeat all of the build-time options so that it just installed whatever was in the stage area regardless of how it was built--not sure how the internal logic works and/or whether such an approach is possible. Thanks |
grafikrobot
previously approved these changes
Feb 28, 2023
ghost
mentioned this pull request
3 tasks
|
Hi @grafikrobot - this is on my stack for today and tomorrow :) |
This was referenced May 9, 2023
Merged
This comment has been minimized.
This comment has been minimized.
ghost
mentioned this pull request
3 tasks
|
|
|
Hi @System-Arch, thanks for your contribution, and thanks @grafikrobot for your guidance! I have tested the local workflow and indeed it now works correctly I've triggered a new build of this PR, as I believe the issues with The removal of Please note that the CLA text hast been updated and contributors need to review the terms and sign it again ahead of merging contributions, thanks!! |
Conan v1 pipeline ✔️All green in build 103 (
Conan v2 pipeline ✔️
All green in build 10 (
|
Hooks produced the following warnings for commit d7fdf5dboost/1.77.0boost/1.81.0boost/1.74.0boost/1.79.0boost/1.80.0boost/1.78.0boost/1.73.0boost/1.71.0boost/1.76.0boost/1.72.0boost/1.75.0 |
|
Hi @System-Arch - this would be ready to be rebuilt and considered for merge. Please note that this cannot be merged until you review the updated terms of the CLA, thanks :) |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
ghost
mentioned this pull request
3 tasks
ghost
mentioned this pull request
3 tasks
This was referenced Aug 13, 2023
ghost
mentioned this pull request
3 tasks
ghost
mentioned this pull request
ghost
mentioned this pull request
3 tasks
|
This pull request has been automatically closed because it has not had recent activity. Thank you for your contributions. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Specify library name and version: boost/1.79.0
Despite claims that this recipe has been ported to Conan 2.0, it is my observation that it does not actually work with Conan 2.0 due to the way that Conan 2.0 manages the package_folder--namely, it is not known at build time; only at package time.
Also, the tests that need access to the GCC libstdc++ runtime lib were failing when using GCC as a tool_requires due to the issue discussed in conan-io/conan#13138