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

ignore pkgconfig when openssl-dir option is specified #486

Merged
merged 1 commit into from
May 27, 2022

Conversation

skaes
Copy link
Contributor

@skaes skaes commented Jan 4, 2022

When openssl is installed on a system with pkconfig files, it is impossible to compile and link against another version of the library.

This patch fixes this by ignoring pkconfig files when the option is given.

@rhenium
Copy link
Member

rhenium commented Jan 4, 2022

Could you show the relevant part of mkmf.log?

mkmf's pkg_config("openssl") is supposed to override PKG_CONFIG_PATH environment variable accordingly if a --with-openssl-{dir,lib} option is specified.

@skaes
Copy link
Contributor Author

skaes commented Jan 4, 2022

Hm. I tried to reproduce the error, but failed. I have to do some research to figure out what I missed. Maybe this was only a problem with rubies before 3.1? Closing the PR for now.

Sorry to have wasted some of your time.

@skaes skaes closed this Jan 4, 2022
@skaes
Copy link
Contributor Author

skaes commented Jan 5, 2022

Under Ruby 3.1.0 openssl does indeed respect the --with-openssl-dir flag correctly, This is due to this commit: ruby/ruby@dff8d12.

However, Ruby 2.7.5 and 3.03 do not respect the flag and will always take whatever pkg-config returns, and this will be the openssl3 library for users of an up to date MacPorts version. Using rvm, 3.0.3 will install and link to the Openssl3 library, which then later causes runtime failures, 2.7.5 will not install at all.

When I created the patch, I was using 3.0.3, and later ported it to 3.1.0 without checking whether the problem had been fixed in the meantime.

What this means is that no rvm user can correctly install any Ruby version before 3.1.0, unless they're using the railsexpress patchsets.

So technically, both the ruby_2_7 and ruby_3_0 branch would need to include the fix I proposed, or they would need the change from ruby/ruby@dff8d12.

How would you like to proceed @rhenium ?

@rhenium
Copy link
Member

rhenium commented Jan 7, 2022

You are right. I just checked it on Ruby 3.0 and it was reading the pkg-config file in /usr instead of the one in --with-openssl-dir.

The reason why this has never caused any issues for me, and probably why I've not seen similar reports before, is because pkg_config("openssl") only takes cflags/-l flags from the result, which is usually just -lcrypto -lssl. I'm guessing MacPorts' OpenSSL 3.0 package specifies something in addition.

Ruby 3.1's behavior is ideal, but it seems more of an improvement than a bug fix. If it were considered a bug fix, Ruby 2.6 (the minimum openssl gem supports) still wouldn't get a backport since it is in security fixes only mode.

I think it makes sense to work around the issue here for Ruby < 3.1. Could you

  • rebase on top of maint-2.1 branch
  • use dir_config() result instead of with_config(), because --with-openssl-dir option may also be replaced by --with-openssl-{include,lib} options

@rhenium rhenium reopened this Jan 7, 2022
@skaes skaes force-pushed the fix-with-openssldir-option branch 3 times, most recently from 4386ad5 to 89745a7 Compare January 11, 2022 14:47
@skaes
Copy link
Contributor Author

skaes commented Jan 11, 2022

@rhenium I have updated the patch as requested, but haven't really had a chance to test it.

@rhenium
Copy link
Member

rhenium commented May 27, 2022

Looks good to me. Thank you!

@rhenium rhenium merged commit e9798b1 into ruby:master May 27, 2022
MSP-Greg added a commit to MSP-Greg/puma that referenced this pull request May 27, 2022
@skaes skaes deleted the fix-with-openssldir-option branch May 27, 2022 16:58
MSP-Greg added a commit to puma/puma that referenced this pull request May 30, 2022
nateberkopec pushed a commit to puma/puma that referenced this pull request Aug 22, 2022
JuanitoFatas pushed a commit to JuanitoFatas/puma that referenced this pull request Sep 9, 2022
jmarrec added a commit to jmarrec/conan-center-index that referenced this pull request Jan 19, 2024
jmarrec added a commit to jmarrec/conan-center-index that referenced this pull request Jan 19, 2024
jmarrec added a commit to jmarrec/conan-center-index that referenced this pull request Jan 22, 2024
jmarrec added a commit to jmarrec/conan-center-index that referenced this pull request Jan 23, 2024
jmarrec added a commit to jmarrec/conan-center-index that referenced this pull request Jan 23, 2024
jmarrec added a commit to jmarrec/conan-center-index that referenced this pull request Jan 23, 2024
jmarrec added a commit to jmarrec/conan-center-index that referenced this pull request Jan 23, 2024
jmarrec added a commit to jmarrec/conan-center-index that referenced this pull request Jan 23, 2024
jmarrec added a commit to jmarrec/conan-center-index that referenced this pull request Jan 23, 2024
jmarrec added a commit to jmarrec/conan-center-index that referenced this pull request Jan 23, 2024
jmarrec added a commit to jmarrec/conan-center-index that referenced this pull request Jan 24, 2024
jmarrec added a commit to jmarrec/conan-center-index that referenced this pull request Feb 2, 2024
jmarrec added a commit to jmarrec/conan-center-index that referenced this pull request Feb 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants