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

fix(openssl): remove renaming of debug files within the recipe #20999

Merged
merged 1 commit into from
Mar 22, 2024

Conversation

Sil3ntStorm
Copy link
Contributor

@Sil3ntStorm Sil3ntStorm commented Nov 8, 2023

Specify library name and version: openssl/*

Removes the "d" suffix which is only ever introduced in conan, without there being any need in a conan environment as different builds are always in different directories.


Copy link
Contributor

github-actions bot commented Nov 8, 2023

🤖 Beep Boop! This pull request is making changes to 'recipes/openssl//'.

👋 @Hopobcn @Croydon you might be interested. 😉

@ghost
Copy link

ghost commented Nov 8, 2023

I detected other pull requests that are modifying openssl/3.x.x recipe:

This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there.

@SpaceIm
Copy link
Contributor

SpaceIm commented Nov 8, 2023

Was there not already a PR for this?

Just for reviewers, be aware that several recipes have fragile workaround for this d postfix introduced by conan, and these recipes may break if it's removed in openssl recipe, like librdkafka since a recent PR: #17894 (comment)

@Sil3ntStorm
Copy link
Contributor Author

There was #10939 but it never got anywhere and the recipe had changed to much. So this is the current version superseding the previous one.

@conan-center-bot

This comment has been minimized.

@uilianries
Copy link
Member

@Sil3ntStorm Could you please explain the case that you are trying to fix and is blocking/breaking you? I'm not confident of changing OpenSSL in case there is no real blocker involved, this recipe affects many others packages in Conan Center.

@ghost
Copy link

ghost commented Nov 18, 2023

I detected other pull requests that are modifying openssl/1.x.x recipe:

This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there.

Copy link
Contributor

@Croydon Croydon left a comment

Choose a reason for hiding this comment

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

In theory we never break end-consumers because everyone is using revisions anyway 😅

And if they don't use revisions then they get breaks all the time from CCI.

This is a change of OpenSSL that we should never have added. It changes the outcome from what upstream intended and the current state might actual be broken for some consumers.

I would rather fix those theoretical cases than avoiding some fixing of other recipes.

I did a quick search for libssl and libcrypto in all recipes, looked at the results and and it looks like only librdkafka needs updating

@jcar87
Copy link
Contributor

jcar87 commented Mar 22, 2024

In theory we never break end-consumers because everyone is using revisions anyway 😅
And if they don't use revisions then they get breaks all the time from CCI.

Indeed. this PR is likely to have issues for Conan 1.x users that do not have revisions enabled. If they don't have revisions enabled and already have Debug binaries for this library, the package info() will be wrong and they won't be able to link.

Even with revisions enabled, downstream consumers are only rebuilt if they use the static library, but won't be rebuilt if they depend on the shared variant.
If changing the .lib filename causes changes to the dll name that will be embedded in a downstream DLL consumer - then this is a breaking change even in that scenario, that would require a rebuild of the consumer package.

Changing the name of the library that gets embedded in the downstream consumers (the name of the .dll or the portion of the library name that gets embedded in the DT_NEEDED or equivalent in Lunux or mac, including the SONAMe) is akin to a breaking ABI change, but Conan cannot know this from the version alone (especially if only the recipe revision changed). This is why these things should never be modified.

This is a change of OpenSSL that we should never have added. It changes the outcome from what upstream intended and the current state might actual be broken for some consumers.

I fully agree. We are going to be more careful and reject any changes like this in future recipes. I've noticed some contributors renaming libraries because it then makes for a "cleaner" implementation of the libs in package_info() - however, the library filenames should not be changed. Arguably, handling this conditionally in the package_info() is equivalent complexity/lines of code. We are going to be more careful as reviews.

I would rather fix those theoretical cases than avoiding some fixing of other recipes.

Agree !

Copy link
Contributor

@jcar87 jcar87 left a comment

Choose a reason for hiding this comment

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

Agree with the changes, but this may actually cause breakages - we are evaluating the risk before merging this

@jcar87
Copy link
Contributor

jcar87 commented Mar 22, 2024

Update: have verified that the DLL names are the same regardless of the name of the import library (.lib). So any issues with revisions would be limited to Conan 1.x users who update the recipe without rebuilding the package, if such package exists.
Will re-run the build to generate the binaries for the most recent versions before merging.

@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ✔️

All green in build 1 (1939d5c1ce2b11ae7a95acd54ab935543ffd1b0b):

  • openssl/1.1.1w:
    All packages built successfully! (All logs)

  • openssl/3.1.3:
    All packages built successfully! (All logs)

  • openssl/3.2.1:
    All packages built successfully! (All logs)

  • openssl/3.2.0:
    All packages built successfully! (All logs)

  • openssl/3.0.12:
    All packages built successfully! (All logs)

  • openssl/3.1.5:
    All packages built successfully! (All logs)

  • openssl/3.1.4:
    All packages built successfully! (All logs)

  • openssl/3.0.13:
    All packages built successfully! (All logs)


Conan v2 pipeline ✔️

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

All green in build 1 (1939d5c1ce2b11ae7a95acd54ab935543ffd1b0b):

  • openssl/1.1.1w:
    All packages built successfully! (All logs)

  • openssl/3.2.1:
    All packages built successfully! (All logs)

  • openssl/3.1.4:
    All packages built successfully! (All logs)

  • openssl/3.1.3:
    All packages built successfully! (All logs)

  • openssl/3.0.13:
    All packages built successfully! (All logs)

  • openssl/3.0.12:
    All packages built successfully! (All logs)

  • openssl/3.2.0:
    All packages built successfully! (All logs)

  • openssl/3.1.5:
    All packages built successfully! (All logs)

@conan-center-bot conan-center-bot merged commit f4b52e8 into conan-io:master Mar 22, 2024
48 checks passed
Ahajha pushed a commit to Ahajha/conan-center-index that referenced this pull request Apr 6, 2024
SpaceIm added a commit to SpaceIm/conan-center-index that referenced this pull request May 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.

8 participants