-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[doxygen] Statically compile libc/libstdc++ to remove host system dependency #18415
[doxygen] Statically compile libc/libstdc++ to remove host system dependency #18415
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
recipes/doxygen/all/conanfile.py
Outdated
if (Version(conan_version).major >= 2): | ||
# Models libc++ compatibility and identifies Debug builds as equivalent to Release builds | ||
compatible_versions = [{"settings": [("compiler.version", v), ("build_type", "Release")]} | ||
for v in self.settings.compiler.version.possible_values() if v <= Version(self.settings.compiler.version)] | ||
return compatible_versions |
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.
To be clear about the intent of this PR - this compatibility()
implementation is not the main purpose of this PR. The main purpose of this PR is to delete the del self.info.settings.compiler
statement in the package_id()
method. The information erasure here hurts the ability for users to manage libcxx abi compatibility.
This compatibility()
implementation is a proposal for consideration of how to improve the model of libcxx compatibility going forward. It is not perfect (in the sense that it doesn't capture the binary compatibility for all binaries that would be compatible), but it does improve upon the representation indended by deleting self.info.settings.compiler
.
Unfortunately the effectiveness of this compatibility()
definition is largely inhibited by restrictions around CCI recipes modifying the package_id
mode. As it stands, this only provides a benefit if a user were to attempt to consume doxygen as follows, assuming that a binary had been created using the gcc10
profile already:
conan install --requires doxygen/1.9.4 -pr:h gcc10 -pr:b gcc10 -s "doxygen/*:compiler.version=gcc12"
This is because, if the user does something like this:
conan install --requires doxygen/1.9.4 -pr:h gcc12 -pr:b gcc12
Then the entire dependency tree utilises gcc12
, and the package id identified for a compatible version of doxygen (built with gcc10) changes.
Given the limited utility of this compatibility()
implementation, I'm happy to remove it if required. It does positively contribute to this recipe, but as above, only in a limited way. I wanted to present this for consideration to trigger some thought about when the compatibility()
method provides value in CCI recipes/what a standard practice should be.
recipes/doxygen/all/conanfile.py
Outdated
@@ -54,10 +54,13 @@ def requirements(self): | |||
self.requires("xapian-core/1.4.19") | |||
self.requires("zlib/1.2.13") | |||
|
|||
def package_id(self): | |||
del self.info.settings.compiler |
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 is the primary purpose of this PR. This kind of statement should be removed from all tool_requires
packages. See option (3) proposed in #17034 for more detail
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.
I don't understand, if it should be removed, and if it's a primary purpose of this PR, then why it's not removed?
P.S. all the side changes could go into another PR. it's good practice to keep PRs atomic and focused on one thing at time. it's easier to review/approve changes if the do only one thing.
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.
I don't understand, if it should be removed, and if it's a primary purpose of this PR, then why it's not removed?
Yeah, I can appreciate this is confusing. The evolution of the review is such that this wasn't identified as the best approach to resolving the glibcxx problems. I called this out here to distinguish it from the compatibility()
implementation which was just a complementary proposal in addition to this. What this turned into is statically compiling glibcxx.
P.S. all the side changes could go into another PR. it's good practice to keep PRs atomic and focused on one thing at time. it's easier to review/approve changes if the do only one thing.
The side changes only became necessary in this PR to get the pipeline working again. Honestly though, given how long it takes to go through a review cycle I'm loathe to split it out as I want to get these changes in. If team review was more responsive id be more inclined to submit more atomic PRs
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.
the best approach I could recommend is to create multiple PRs, each one for its own set of changes, e.g.:
- PR 1 - solution 1 (remove
del self.info.settings.compiler
) - PR 2 - solution 2 (add
compatibility()
method) - PR 3 - solution 3 (link
libstdc++
statically) - PR 4 - side changes (e.g. libiconv changes)
then it would be much more easier to read and review every solution. right now, I am sorry to say, but PR is a bit chaotic and hard to review, as PR title, comments in code and comments in review do not match each other, confusing reviewers on what's actually happening here (like comment here says "X" is the main thing in PR, but PR doesn't include "X", and instead includes some unrelated "Y" and "Z", which is mind-blowing).
This comment has been minimized.
This comment has been minimized.
Hi @samuel-emrys - thank you for raising this PR and for your detailed investigation and explanation. From what I can see, there are two issues that are highly related but not exactly the same:
Here there are multiple solutions to consider. For the (1) problem, ignoring (2) altogether - preventing the deletion of For (1), what we have done in other recipes, most recently in For (2), we could probably relax the condition, and prevent the full package reference from ending up in the package info of
I see this is inline with what you've previously proposed - having reviewed the situation and having a better understanding within the team, this may be an acceptable change. I think the combination of static C++ runtime and using the
^ on Linux/x86_64 this would download the binaries from the Conan Center remote, and they would run, assuming the gcc11 runtime is in the system For any other version of gcc in the profile, e.g.:
This would fail with missing binaries, as you point out above. With the changes I'm proposing:
it would always retrieve same package ID from conan Center, regardless of version of gcc, and it would always run on versions that have even older gcc, provided the glibc version is at least the one used by Conan Center (we still target an older glibc in Conan Center, so this should be fine). |
Yes, exactly.
Yes, this might be true as well. I think your reasoning here makes sense, but it's not what I've observed. The observation I made impacted the ability for the To address your proposed solution: Static C++ runtime + relax CCI requirements on
|
Modes / Variables | name | version | user | channel | package_id | RREV | PREV |
---|---|---|---|---|---|---|---|
minor_mode() | Yes | Yes (e.g., 1.2.Z+b102) | No | No | No | No | No |
full_version_mode() | Yes | Yes (e.g., 1.2.3+b102) | No | No | No | No | No |
The difference between these two appears to be mostly whether you're assuming that the library uses semver or not. I wasn't able to identify a versioning strategy for doxygen
, so would full_version_mode()
be more appropriate here?
Anyway, happy to proceed either way - both solve my issue, but my preference would be for what's in this PR. It would also be good to establish a practice across CCI for this kind of thing so that there's broad awareness of the strategy.
This is interesting! Any information you could provide for us to check, would be greatly appreciated.
The first part is correct - it throws away information in the package_id, but I'd say this is intentional as explained above, based on the knowledge that for this particular case, these are less relevant: our goal here is to broaden the scenarios in which a single package id can satisfy the requirement, such that consumers don't have to build it from source when they use newer versions of compilers. This saves time, and a new version of a compiler may not add any value to the executable. For something consumed as an executable application, the version of the compiler si no longer as relevant as for a library (where the version of the compiler may play a part in ABI compatibility) The second statement I think it wouldn't be as accurate. On the one hand, the compatibility of the runtimes (glibc, libstdc++) is external to the recipe, and with the For what it's worth, I think the logic you are proposing by implementing the Crucially, I think that this means that users can customize this to their environments without changes to the recipes - that is, a Conan 2.0 can customize both the |
One additional consideration, and an argument against a When we use clang on Linux with GNU's libstdc++, e.g:
Clang will, by default, use the most recent gcc toolchain it finds on the system, as per the docs: https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-gcc-toolchain (adding This can create an interesting situation. I can use, say, clang-13 on a Linux distro with gcc-12, and the same |
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Conan 1.x doesn't have the necessary SettingsItem.get_definition() to query settings.yml, so guard it to enable a conan 2.0 implementation
…mpatibility * Use possible_values() to retrieve the total set of possible compiler versions rather than the deprecated/private get_definition for compatibility with conan > 2.0.5 * Remove package_id modification for CCI compatibility.
This reverts commit 2b96556.
4c61f7e
to
a7ca12a
Compare
Conan v1 pipeline ❌Failure in build 9 (
Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability. |
Notes for myself for future debugging:
These are not equivalent because the build type applies to the entire dependency tree, and the dependencies influence the package id of doxygen. With the existing compatibility method, the following are equivalent:
This means that we expect to see 1x Release build and 1x Debug build in the c3i jobs as they currently stand.
This indicates glibc is not being compiled statically. Looking at the log, it's matching the prebuilt
which also indicates that
This indicates that |
@jcar87 @uilianries @RubenRBS if any of you have any guidance you can provide on how you've statically compiled glibc in in the past, that would be appreciated. I had a quick look through the CCI recipes and nothing that I'm not already doing jumped out at me. |
The other element to this is whether we should even try to statically link libc in this case - as above, there are dangers in doing so, and the original issue of GLIBCXX compatibility is solved by statically linking libstdc++. Given the approach that conan-center-index takes to manage glibc compatibility, I'm confused as to why this is an issue at all - shouldn't all of these tests be built with ubuntu 18.04? Ubuntu 18.04 has a glibc version of 2.27, so where is the glibc 2.29 requirement coming from? |
@jcar87 @uilianries @RubenRBS any help would be appreciated. I fear this is an infra issue and I have no insight into infra config |
@samuel-emrys thanks for the ping! I have scheduled a review with Uilian on monday so we can both look into this, thanks a lot for your patience :) |
To be clear about the issue here, given I realise my comment above (to track my debugging) was a bit meandering:
Is the solution to make sure that doxygen is rebuilt with an earlier GLIBC version? How can we ensure that that will be the case? |
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. |
@RubenRBS @uilianries any ideas here? How did your discussion go? |
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. |
@RubenRBS reminder to take a look at this when you get a chance |
@RubenRBS @jcar87 @uilianries any guidance on how to progress this would be appreciated. |
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. |
This pull request has been automatically closed because it has not had recent activity. Thank you for your contributions. |
This is a successor PR to #17051.
self.info.settings.compiler
because this is relevant metadata for executables, as it inserts a dependency on a libstdc++ version at least what it was compiled with. For a detailed explanation of the problem this solves and why it's necessary please read [service] Removingcompiler
frompackage_id
causes libstdc++ compatibility errors in tool_requires packages #17034The previous form of this PR modified the
package_id
to usefull_version_mode
to ensure that the dependency package id's don't impact doxygen's package id - only version changes in dependencies would impact doxygen's package id. This was necessary to allow doxygen built with gcc 10 to be identified as compatible with a gcc 12 profile. Without this,compiler.version=12
propagates through the entire dependency tree and the package id of the doxygen package built with gcc 10 does not match appropriately and is not resolved as compatible, since it is impacted by the varying package_id of its upstream dependencies.I've removed this as this was the reason #17051 was rejected, but I do note that it does hamper the existing implementation of
compatibility()
substantially as the benefits are only really seen when the only package in doxygen's tree using a different compiler version is doxygen.Relates to #17034