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

Issue with 'automatic' compatible packages and custom package ids #16772

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

harsszegi
Copy link

@harsszegi harsszegi commented Aug 2, 2024

ISSUE-16785: Issue with 'automatic' compatible packages and custom package ids

In case the recipe uses its own package_id() and compatible packages are generated automatically (e.g. Visual Studio vs msvc), then the compatible packages will use wrong 'base' info set for the compatible package id generation.

In case the recipe's own package_id() erases a few options / settings, the compatible packages will not know about it, as those are generated prior the recipe's package_id() is being called.

The change makes sure that first the recipe's package_id() is called, then the compatible packages are generated, so that they pick up the updated info set.

Changelog: (Bugfix): Automatically generated compatible packages are wrong in case of custom package_id()
Docs: Omit

  • Refer to the issue that supports this Pull Request.
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

In case the recipe uses its own package_id() and compatible packages
are generated automatically (e.g. Visual Studio vs msvc), then the compatible
packages will use wrong 'base' info set for the compatible package id
generation.

In case the recipe's own package_id() erases a few options / settings,
the compatible packages will not know about it, as those are generated prior
the recipe's package_id() is being called.

The change makes sure that first the recipe's package_id() is called, then
the compatible packages are generated, so that they pick up the updated
info set.
@harsszegi
Copy link
Author

@memsharded I've ran into a ''somewhat'' complex issue with regards to the msvc / Visual Studio compatibility :(

Currently "automatic" compatible_packages are generated prior calling the package_id().
This has a drawback though that if the package_id() erases this-and-that, then the compatible packages will look for wrong packages (as those still contain the "this-and-that" part of the options/settings).

I have tested locally and fixes my use-case, though I have no clue how intrusive the change is :(
Please advise,
Thanks,

Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution.

I honestly don't know what would be the best. On one hand, this looks risky. It is likely that this will be breaking someone relying on the specific order of operations. I know the tests are not breaking, but the coverage for this "msvc-compatible" feature is a bit too shallow, I don't think it can be fully trusted.

Also, we no longer have the resources to deeply investigate this for Conan 1.

On the other hand, most users are staying with Visual Studio in Conan 1 and moving to msvc in profiles in Conan 2, as recommended, because we are aware that there are issues, so it is much smoother to stay using in Visual Studio in Conan 1. We have done it at scale in ConanCenter, and I think users are using this approach, so hopefully not many users would be hit by possible unexpected side effects of this change.

@harsszegi
Copy link
Author

harsszegi commented Aug 9, 2024

Hi @memsharded,

Thanks for the reply. I see the following use cases:

  • There is no package_id() in the recipe or the package_id() doesn't alter self.info (which is kinda strange on its own, as why would you have a package_id() if it doesn't do actually anything with self.info). In this case the changes are irrelevant, as the compatibility layer will work on the exact same self.info as before
  • There is a package_id() and it alters self.info but the recipe doesn't use 'msvc' by any means. In this case the changes are also irrelevant (as the msvc compatible package logic is not triggered).
  • There is a package_id() and it alters self.info and the recipe uses 'msvc'. In this case the PR is mandatory as it fixes a currently buggy behavior
  • There is a package_id() and it uses for some strange reason self.compatible_packages to do something with self.info. In this case the PR will break this recipe.

Sadly I have zero idea how realistic the last use case is :)

@memsharded
Copy link
Member

I am afraid that it will not be possible to move this forward. I have added to your PR a simple test:

def test_crash():
    client = TestClient()
    conanfile = textwrap.dedent("""
       from conans import ConanFile

       class Pkg(ConanFile):
           settings = "os", "compiler"
           def package_id(self):
               del self.info.settings.compiler.version
       """)
    profile = textwrap.dedent("""
       [settings]
       os = Windows
       compiler=msvc
       compiler.version=192
       compiler.runtime=dynamic
       compiler.cppstd=14
       build_type=Release
       """)
    client.save({"conanfile.py": conanfile,
                 "myprofile": profile})
    client.run("create . pkg/0.1@ -pr=myprofile")

It will crash because of your change (it works fine in develop branch).
It is very likely that users will have a package_id removing something like del self.info.settings.compiler.version that will make it fail.

@harsszegi
Copy link
Author

harsszegi commented Aug 13, 2024

@memsharded Let me argue with that conclusion of yours a bit :)

Afaik the test should be doing this:

  • you create a package with "msvc" "compiler.version=193"
  • a compatibility package with "Visual Studio" will be created including the corresponding "compiler.version=17"
  • you erase "compiler.version" from your package indicating that it should be compatible with "compiler.version=192" (and compiler.version=16) as well
  • you try to consume that package with a "Visual Studio" profile that has "compiler.version=16" set, it will fail as within the compatible package automatically added with msvc_compatible you'll still have "compiler.version=17" explicitly set)
  • surely you can consume it with "msvc" and with "compiler.version=192" (because of the package_id() behavior)

What do I miss?

[Anyhow added a commit, that checks if a certain compiler.* variable is present at all]

In case a custom package_id() erased some of the compiler settings for
'msvc', a 'Visual Studio' equivalent should be created that also
doesn't have the (possibly) erased elements.
Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

I think that this might be moved forward, still some notes:

  • The release cadence of 1.X is now much slower, I am assigning this to 1.66, but it might take a while to release
  • I think there are still some (relatively minor, but still some) risks to it, maybe due to a bit extreme usage of Conan. But if there is anything breaking or users reporting any issues, this will be reverted.

@memsharded memsharded added this to the 1.66.0 milestone Aug 14, 2024
@harsszegi
Copy link
Author

harsszegi commented Aug 14, 2024

Understood, thanks a lot! Just a quick recap, what would be the release date (roughly) for 1.66? (Our company is blocked from transitioning to Conan 2 without this - obviously I can hack it around via using custom package_id() functions that add another msvc_compatible compatible package with the proper info)

@harsszegi harsszegi closed this Aug 14, 2024
@harsszegi
Copy link
Author

I have absolutely no idea how /why Github has closed the PR :(, reopening it

@harsszegi harsszegi reopened this Aug 14, 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.

3 participants