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

vendor/open-simulation-interface: Add full_package_mode #244

Merged

Conversation

fgguidi
Copy link

@fgguidi fgguidi commented Jun 19, 2024

Set full_package_mode for Protobuf to consider the case that a consumer recipe might set Protobuf as a shared or static library.

How to reproduce issue:

  • Set the following options in your consumer: 'self.options['open-simulation-interface'].shared = True' and
    'self.options['protobuf'].shared = True'
  • Then upload the packages to artifactory.
  • Now change self.options['protobuf'].shared = False in your consumer package.
  • Try to compile again. Assuming that your Conan configuration has as default `default_package_id_mode = semver_direct_mode' then the first version of OSI produced which links to a Protobuf as shared library is installed in your Conan cache.

Set full_package_mode for Protobuf to consider the case that a consumer
recipe might set Protobuf as a shared or static library.

How to reproduce issue:

- Set the following options in your consumer:
'self.options['open-simulation-interface'].shared = True'
and
'self.options['protobuf'].shared = True'
- Then upload the packages to artifactory.
- Now change `self.options['protobuf'].shared = False` in your consumer
package.
- Try to compile again. Assuming that your Conan configuration has as
default `default_package_id_mode = semver_direct_mode' then the first
version of OSI produced which links to a Protobuf as shared library is
installed in your Conan cache.
@fgguidi fgguidi requested review from cassava and tobifalk as code owners June 19, 2024 13:01
Copy link
Contributor

@cassava cassava left a comment

Choose a reason for hiding this comment

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

Part of the goal of these vendor(ed) packages is to be as close as possible to the recipes that are used on Conan Center. This allows us to use the CCI versions as soon as they are sufficient. For a while, we had this for all vendored packages.

Adding this full_package_mode in the package_id method here is nice, but it is a deviation from the way that packages are written in the CCI:

https://github.com/conan-io/conan-center-index/blob/master/recipes/open-simulation-interface/all/conanfile.py

If you rely on it, things will break again if we want to revert to the CCI version again. It seems Conan expects you to define something rational in the conan.conf, (which can be part of the configuration you are expected to be installing when deploying Conan in an organization):

https://docs.conan.io/1/creating_packages/define_abi_compatibility.html#changing-the-default-package-id-mode

Also note the comment on Conan v2:

https://docs.conan.io/1/creating_packages/define_abi_compatibility.html#enabling-full-transitivity-in-package-id-modes

So it seems to me that the better course of action would be to ensure that conan.conf is set correctly for all consumers. The defaults for Conan v1 are anyway not recommended to be used for a while, meaning the effort needs to be done one way or another.

Do you agree with the logic?

@cassava
Copy link
Contributor

cassava commented Jun 27, 2024

That all said: Merging this PR isn't going to break anything so I'm not against it. You have to address the underlying issue too otherwise something will break again in the future.

@fgguidi
Copy link
Author

fgguidi commented Jul 2, 2024

That all said: Merging this PR isn't going to break anything so I'm not against it. You have to address the underlying issue too otherwise something will break again in the future.

I understand your point and I agree that changing this recipe is then not the permanent solution for our issue. I will proceed to merge this PR just to signal this issue for the future and that we eventually have something in the history that can help if it happens again but we will not depend on this solution.
I have temporarily fixed the issue without having to depend on the fixed recipe.

@tobifalk tobifalk merged commit d5f3314 into eclipse:master Jul 2, 2024
6 checks passed
@cassava cassava added this to the 0.25.0 milestone Jul 15, 2024
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.

4 participants