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.

Sign up for GitHub

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

Jump to bottom

[doxygen] Model libstdc++ compatibility #17051

Closed
wants to merge 3 commits into from
Closed

[doxygen] Model libstdc++ compatibility #17051

wants to merge 3 commits into from

Conversation

samuel-emrys
Copy link
Contributor

  • Add model of libstdc++ compatibility to ensure that binaries can not be used with older version of libstdc++ than what it was compiled for.
  • Remove deletion of 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.
  • Modify the dependency version impact on package id to use full_version_mode. This ensures that the dependency package id's don't impact doxygen's package id - only version changes in dependencies will impact doxygen's package id. This is 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 dependency tree and the package id of the doxygen package built with gcc 10 does not match appropriately and is not resolved as compatible.

Relates to #17034

@ghost ghost mentioned this pull request Apr 17, 2023
Closed
@ghost
Copy link

ghost commented Apr 17, 2023 •
edited by ghost
Loading

I detected other pull requests that are modifying doxygen/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 ghost mentioned this pull request Apr 17, 2023
Closed
3 tasks
@prince-chrismc prince-chrismc mentioned this pull request Apr 17, 2023
Open
@conan-center-bot

This comment has been minimized.

Sign in to view
@samuel-emrys
[doxygen] Model libstdc++ compatibility
517198b 
* Add model of libstdc++ compatibility to ensure that binaries can not
  be used with older version of libstdc++ than what it was compiled for.
* Remove deletion of 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.
* Modify the dependency version impact on package id to use
  full_version_mode. This ensures that the dependency package id's don't
  impact doxygen's package id - only version changes in dependencies
  will impact doxygen's package id. This is 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
  dependency tree and the package id of the doxygen package built with
  gcc 10 does not match appropriately and is not resolved as compatible.

Relates to conan-io#17034
@samuel-emrys samuel-emrys force-pushed the recipe/17034-doxygen-libcpp-compatibility branch from 4287699 to 517198b Compare April 17, 2023 14:01
@conan-center-bot

This comment has been minimized.

Sign in to view
@samuel-emrys samuel-emrys mentioned this pull request Apr 17, 2023
Open
@samuel-emrys samuel-emrys mentioned this pull request Apr 30, 2023
Closed
1 task
@samuel-emrys
[doxygen] Improve compatibility definition
3eacdec 
* Improve compatibility definition by removing the need to manually
  query settings.yml, instead using get_definition() to provide the
  possible values of a SettingsItem.
@conan-center-bot

This comment has been minimized.

Sign in to view
@samuel-emrys
[doxygen] Guard compatibility definition for conan 2
d0b1762 
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
@conan-center-bot

This comment has been minimized.

Sign in to view
@conan-center-bot

This comment has been minimized.

Sign in to view
@conan-center-bot

This comment has been minimized.

Sign in to view
uilianries
uilianries reviewed May 18, 2023
View reviewed changes
recipes/doxygen/all/conanfile.py
@@ -55,9 +57,17 @@ def requirements(self):
self.requires("zlib/1.2.13")

def package_id(self):
del self.info.settings.compiler
self.info.requires.full_version_mode()
Copy link
Member

Choose a reason for hiding this comment

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

Please, do not enforce another package version mode, besides the default one. This is break the dependencies graph and will result in missing packages.

uilianries
uilianries requested changes May 18, 2023
View reviewed changes
Copy link
Member

@uilianries uilianries left a comment

Choose a reason for hiding this comment

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

custom package id mode breaks cci

@conan-center-bot

This comment has been minimized.

Sign in to view
@conan-center-bot

This comment has been minimized.

Sign in to view
@ghost ghost mentioned this pull request Jun 27, 2023
Closed
3 tasks
@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ✔️

All green in build 9 (d0b17626b86fa44d8dd69af23ca391aa9078cda2):

  • doxygen/1.9.2@:
    All packages built successfully! (All logs)

  • doxygen/1.9.1@:
    All packages built successfully! (All logs)

  • doxygen/1.8.17@:
    All packages built successfully! (All logs)

  • doxygen/1.9.4@:
    All packages built successfully! (All logs)

  • doxygen/1.8.18@:
    All packages built successfully! (All logs)

  • doxygen/1.8.20@:
    All packages built successfully! (All logs)

Conan v2 pipeline ✔️

Note: Conan v2 builds may be required once they are on the v2 ready list

All green in build 4 (d0b17626b86fa44d8dd69af23ca391aa9078cda2):

  • doxygen/1.9.2@:
    All packages built successfully! (All logs)

  • doxygen/1.9.1@:
    All packages built successfully! (All logs)

  • doxygen/1.9.4@:
    All packages built successfully! (All logs)

  • doxygen/1.8.20@:
    All packages built successfully! (All logs)

  • doxygen/1.8.18@:
    All packages built successfully! (All logs)

  • doxygen/1.8.17@:
    All packages built successfully! (All logs)

@uilianries
Copy link
Member

This pull request may not be merged, as it will break CCI due a custom package ID not used by CI. The result will be missing packages, incapacitating other PRs of using doxygen. Thank you for your effort and case report, it's a valid situation, but can not be fixed with that side-effect.

@uilianries uilianries closed this Jun 28, 2023
@samuel-emrys
Copy link
Contributor Author

samuel-emrys commented Jun 28, 2023 •
edited
Loading

@uilianries can you elaborate on this? The whole purpose of this pr is to make the package id more flexible to ensure that doxygen is rebuilt as necessary. This is an extremely important consideration for tool requires, because the current method CCI employs actually does result in broken binaries for any compiler less than gcc11.

Features were added to Conan to support this explicitly in #13793. I was waiting for the CCI version of Conan to catch up before revising this pr. I think the full_package_id() might be removable if the same compatibility change is made to xapian-core, but I'd have to check this

@samuel-emrys samuel-emrys mentioned this pull request Jul 8, 2023
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@uilianries uilianries uilianries requested changes

@jcar87 jcar87 Awaiting requested review from jcar87

@danimtb danimtb Awaiting requested review from danimtb

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@samuel-emrys @conan-center-bot @uilianries