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

autoconf: Do not clear "os" from package info #25786

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

cubanpit
Copy link

@cubanpit cubanpit commented Nov 1, 2024

Avoids compatibility issues between Windows and Linux.

Summary

Changes to recipe: autoconf

Motivation

Not including os information in the package means that packages built on Linux and Windows mix up, with same package ID and different package revisions. The most recent one will then be pulled from the remote and used, but they might actually not be fully compatible between OSes. In our case the Windows-built version did not have the executable flag set on the scripts in bin/, this caused issues when using the package in a Linux environment.
The symptoms were similar to conan-io/conan#14345, so we initially thought the problem happened in decompressing the package files.

Details

Just keep the os information, it causes minimal duplication and it makes sure the package works in all environments.


Avoids compatibility issues between Windows and Linux.
@CLAassistant
Copy link

CLAassistant commented Nov 1, 2024

CLA assistant check
All committers have signed the CLA.

@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ✔️

Warning

Conan Center will stop receiving updates for Conan 1.x packages soon - please see announcement.

All green in build 1 (f2ad7f894cde60c13e7bddcb8918d72296a6b8f2):

  • autoconf/2.72:
    Built 5 packages out of 11 (All logs)

  • autoconf/2.71:
    Built 5 packages out of 11 (All logs)


Conan v2 pipeline ✔️

Note: Conan v2 builds are now mandatory. Please read our discussion about it.

All green in build 1 (f2ad7f894cde60c13e7bddcb8918d72296a6b8f2):

  • autoconf/2.71:
    Built 4 packages out of 5 (All logs)

  • autoconf/2.72:
    Built 4 packages out of 5 (All logs)

uilianries
uilianries previously approved these changes Nov 7, 2024
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.

Good catch!

@@ -38,7 +38,9 @@ def requirements(self):
self.requires("m4/1.4.19") # Needed at runtime by downstream clients as well

def package_id(self):
self.info.clear()
del self.info.settings.arch
Copy link
Member

Choose a reason for hiding this comment

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

The self.info.clear() also clears:

  • Possible variability from options, not an issue in this case
  • Possible variability from dependencies. In this case, it could be a problem as self.requires("m4/1.4.19") is a requirement, and changing that version will create another package_id. Most like the self.info.requires.clear() is necessary too

Copy link
Author

Choose a reason for hiding this comment

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

I understand and I added this change to the branch.
I would imagine that a new version of m4 can change the output, but then the package revision would change as well.
A change in the version of m4 would also mean a change in the recipe revision though, so a new package would be generated anyway, does this actually avoid duplication?

To avoid generating a new package ID for each new version of the requirements.
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.

5 participants