OpenSSL 1.x.x Conan 2.0 compatibility #14066
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
mentioned this pull request
|
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-prsso don't hesitate to report issues/improvements there. |
This was referenced Nov 6, 2022
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit aed24c8openssl/1.1.1qopenssl/1.1.1popenssl/1.1.1sopenssl/1.0.2u |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit 4480966openssl/1.1.1sopenssl/1.1.1popenssl/1.0.2uopenssl/1.1.1qopenssl/1.1.0l |
|
Hi @prince-chrismc, I believe the error quoted below and shown in conan-center-bot comments above is faulty as this recipe has been tested extensively with Conan 2.0-beta4 including on a Mac. I suspect the bot is using an older Conan 2 beta that lacks support for XCRun in conan.tools.apple.
|
|
Yep.... $ docker run -it conanio/gcc12-ubuntu16.04:2.0.0-pre
Unable to find image 'conanio/gcc12-ubuntu16.04:2.0.0-pre' locally
2.0.0-pre: Pulling from conanio/gcc12-ubuntu16.04
58690f9b18fc: Pull complete
b51569e7c507: Pull complete
...
8248de24211b: Pull complete
Digest: sha256:db3a75857d6edbbc95d638b7e5ee75f0b98c795277ace5fa38d904ab9eebaebe
Status: Downloaded newer image for conanio/gcc12-ubuntu16.04:2.0.0-pre
conan@82be81d87aaf:~$ conan --version
Conan version 2.0.0-beta3
conan@82be81d87aaf:~$I'll flag this to the team |
prince-chrismc
added
the
infrastructure
1 task
Co-authored-by: Chris Mc <[email protected]>
Co-authored-by: Chris Mc <[email protected]>
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
This is some absolutely amazing work! The fact such an old recipe is working with both 1.x and 2.0 is a huge feat!
That being said, there's a lot of "bad smell" that we thing it's time to address. For example the usage of finding system tools is something we know is problematic. The usage of "find make" is definitely not what we want to be seeing with the conf options.
We know this would be a huge undertaking, so one of us (probably that noob @prince-chrismc) to almost start from scratch. However I do want your input on how'd you like to proceed.
I think we can go ahead with the merging this PR is the few new comments I have added are address but I absolutely do not mind taking this over in a much large refactor.
It's been years and even I am scared to touch this recipe but there are long standing issues like #3476 that I would love to give some attention
What would be a good next step for you? I would really like to continue with what you've done ❤️
recipes/openssl/1.x.x/conanfile.py
Outdated
| else: | ||
| lib_path = zlib_info.lib_paths[0] # Just path, linux will find the right file | ||
| zlib_cpp_info = self.dependencies["zlib"].cpp_info |
There was a problem hiding this comment.
So the conan_version is always something that catches attention, I brought this one up to the team and we discussed if this was the "current best approach" (obviously a moving target - what you did is absolutely correct 👍 we'd just like to set a baseline for others who come after)
This particular line should work in both 1.x and 2.0 - it should be use in the generate() save some files and then be read from the build() (this is ho the Toolchains` work for comparison)
We'd really love to see that approach used here in this case as well as we things it's the best solution
There was a problem hiding this comment.
Hi @prince-chrismc,
I implemented what I think you were suggesting here. Let me know if it is okay now.
Co-authored-by: Chris Mc <[email protected]>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
recipes/openssl/1.x.x/conanfile.py
Outdated
| def _make_program(self): | ||
| if self._use_nmake: | ||
| return "nmake" | ||
| make_program = os.getenv("CONAN_MAKE_PROGRAM") or shutil.which("make") or shutil.which('mingw32-make') |
There was a problem hiding this comment.
👍 for future readers, this is replaced by the conf tools.gnu:make_program https://docs.conan.io/en/latest/reference/config_files/global_conf.html?highlight=make%20program#tools-configurations with the new generators
| @@ -7,7 +7,7 @@ | |||
| #include <openssl/crypto.h> | |||
| #include <openssl/ssl.h> | |||
| #if defined(WITH_ZLIB) | |||
| #include <zlib.h> | |||
There was a problem hiding this comment.
This is needed as in 2.0 the graph does not expose transitive header or libs which is why the feature test through openssl is required
There was a problem hiding this comment.
Hi @prince-chrismc,
The original test here did not actually validate that OpenSSL was built with zlib functionality. I revised the test to confirm that the zlib support was present in OpenSSL. This support does not require client access to the zlib header file. Let me know if you have any questions or concerns. Thanks
| if not self.options.no_asm and not tools.which("nasm"): | ||
| self.build_requires("nasm/2.15.05") | ||
| if self._win_bash and not tools.get_env("CONAN_BASH_PATH"): | ||
| if not self.win_bash: |
There was a problem hiding this comment.
This is the new way for future readers https://docs.conan.io/en/latest/migrating_to_2.0/recipes.html?highlight=win_bash#windows-subsystems
|
|
||
| def generate(self): | ||
| tc = AutotoolsToolchain(self) | ||
| # workaround for random error: size too large (archive member extends past the end of the file) |
There was a problem hiding this comment.
| args = [ | ||
| '"%s"' % (self._target if self._full_version >= "1.1.0" else self._ancestor_target), | ||
| "shared" if self.options.shared else "no-shared", | ||
| "--prefix=\"%s\"" % prefix, | ||
| "--prefix=/", |
There was a problem hiding this comment.
This jumped out at me, this was the package folder but now it's just root 🤔 Is the new Autotools install doing a better job?
There was a problem hiding this comment.
Hi @prince-chrismc,.
It is my understanding that in Conan 2.,0 self.package_folder is determined dynamically in the export() method and is not known when the generate() or build() methods are called. This is the pattern I have been using to permit build artifacts to be relocated at export-pkg time. Let me know if I am missing anything. Thanks.
There was a problem hiding this comment.
Perfect this address a bunch of the the questions/suggestions/concerns! There's a lot less code smell ❤️
I have one technical questions https://github.com/conan-io/conan-center-index/pull/14066/files#r1052713668 and one tiny nit about a comment https://github.com/conan-io/conan-center-index/pull/14066/files#r1052707402
Overall it looks super good, just want to make sure I understand the changes
Co-authored-by: Chris Mc <[email protected]>
Co-authored-by: Chris Mc <[email protected]>
Conan v1 pipelineAll green in build 21 (
|
prince-chrismc
approved these changes
Dec 22, 2022
czoido
approved these changes
Dec 23, 2022
SpaceIm
reviewed
Dec 23, 2022
Comment on lines
232
to
239
| def build_requirements(self): | ||
| if self._settings_build.os == "Windows": | ||
| if not self._win_bash: | ||
| self.build_requires("strawberryperl/5.30.0.1") | ||
| if not self.options.no_asm and not tools.which("nasm"): | ||
| self.build_requires("nasm/2.15.05") | ||
| if self._win_bash and not tools.get_env("CONAN_BASH_PATH"): | ||
| if not self.win_bash: | ||
| self.tool_requires("strawberryperl/5.30.0.1") | ||
| if not self.options.no_asm: | ||
| self.tool_requires("nasm/2.15.05") | ||
| if self.win_bash and not os.getenv("CONAN_BASH_PATH") and not self._use_nmake: | ||
| self.build_requires("msys2/cci.latest") |
There was a problem hiding this comment.
I've not tested but I'm pretty sure it breaks MinGW. There is a confusion here about self.win_bash, it's not something set externally.
We have now a well established pattern to handle win bash in conan v2, not sure to understand why it has not been used here.
There was a problem hiding this comment.
I confirm, it's broken:
> conan install openssl/1.1.1s@ -o openssl:shared=True -pr:b win-x86_64-gcc12 -pr:h win-x86_64-gcc12 -b missing
Configuration (profile_host):
[settings]
arch=x86_64
build_type=Release
compiler=gcc
compiler.exception=seh
compiler.libcxx=libstdc++11
compiler.threads=posix
compiler.version=12
os=Windows
[options]
openssl:shared=True
[build_requires]
[env]
CC=D:/mingw64/bin/gcc.exe
CXX=D:/mingw64/bin/g++.exe
[conf]
tools.cmake.cmaketoolchain:generator=Ninja
tools.build:compiler_executables={'c': 'D:/mingw64/bin/gcc.exe', 'cpp': 'D:/mingw64/bin/g++.exe'}
[buildenv]
CC=D:/mingw64/bin/gcc.exe
CXX=D:/mingw64/bin/g++.exe
Configuration (profile_build):
[settings]
arch=x86_64
build_type=Release
compiler=gcc
compiler.exception=seh
compiler.libcxx=libstdc++11
compiler.threads=posix
compiler.version=12
os=Windows
[options]
openssl:shared=True
[build_requires]
[env]
CC=D:/mingw64/bin/gcc.exe
CXX=D:/mingw64/bin/g++.exe
[conf]
tools.cmake.cmaketoolchain:generator=Ninja
tools.build:compiler_executables={'c': 'D:/mingw64/bin/gcc.exe', 'cpp': 'D:/mingw64/bin/g++.exe'}
[buildenv]
CC=D:/mingw64/bin/gcc.exe
CXX=D:/mingw64/bin/g++.exe
openssl/1.1.1s: Not found in local cache, looking in remotes...
openssl/1.1.1s: Trying with 'conancenter'...
Downloading conanmanifest.txt completed [0.18k]
Downloading conanfile.py completed [41.44k]
Downloading conan_export.tgz completed [0.33k]
Decompressing conan_export.tgz completed [0.00k]
openssl/1.1.1s: Downloaded recipe revision c5843f5e0776c9e862e08c911cd09958
strawberryperl/5.30.0.1: Not found in local cache, looking in remotes...
strawberryperl/5.30.0.1: Trying with 'conancenter'...
Downloading conanmanifest.txt completed [0.10k]
Downloading conanfile.py completed [2.25k]
Downloading conan_export.tgz completed [0.31k]
Decompressing conan_export.tgz completed [0.00k]
strawberryperl/5.30.0.1: Downloaded recipe revision d125df083747d815c66e9ee621f3909f
nasm/2.15.05: Not found in local cache, looking in remotes...
nasm/2.15.05: Trying with 'conancenter'...
Downloading conanmanifest.txt completed [0.22k]
Downloading conanfile.py completed [4.04k]
Downloading conan_export.tgz completed [0.31k]
Decompressing conan_export.tgz completed [0.00k]
nasm/2.15.05: Downloaded recipe revision c13d1db156147946c9aae400891b4baa
Installing package: openssl/1.1.1s
Requirements
openssl/1.1.1s from 'conancenter' - Downloaded
Packages
openssl/1.1.1s:bb11550f8236e219d08dd4d948c91d66592f7318 - Build
Build requirements
nasm/2.15.05 from 'conancenter' - Downloaded
strawberryperl/5.30.0.1 from 'conancenter' - Downloaded
Build requirements packages
nasm/2.15.05:0a420ff5c47119e668867cdb51baff0eca1fdb68 - Download
strawberryperl/5.30.0.1:ca33edce272a279b24f87dc0d4cf5bbdcffbc187 - Download
Installing (downloading, building) binaries...
nasm/2.15.05: Retrieving package 0a420ff5c47119e668867cdb51baff0eca1fdb68 from remote 'conancenter'
Downloading conanmanifest.txt completed [0.73k]
Downloading conaninfo.txt completed [0.32k]
Downloading conan_package.tgz completed [2044.13k]
Decompressing conan_package.tgz completed [0.00k]
nasm/2.15.05: Package installed 0a420ff5c47119e668867cdb51baff0eca1fdb68
nasm/2.15.05: Downloaded package revision 801ea6b6d3c17d507832dc2f9c17b6b8
strawberryperl/5.30.0.1: Retrieving package ca33edce272a279b24f87dc0d4cf5bbdcffbc187 from remote 'conancenter'
Downloading conanmanifest.txt completed [483.04k]
Downloading conaninfo.txt completed [0.30k]
Downloading conan_package.tgz completed [32402.67k]
Decompressing conan_package.tgz completed [0.00k]
strawberryperl/5.30.0.1: Package installed ca33edce272a279b24f87dc0d4cf5bbdcffbc187
strawberryperl/5.30.0.1: Downloaded package revision ffd124eb48d535d9e5b7134311ef0b65
openssl/1.1.1s: Applying build-requirement: strawberryperl/5.30.0.1
openssl/1.1.1s: Applying build-requirement: nasm/2.15.05
Downloading conan_sources.tgz completed [0.41k]
Decompressing conan_sources.tgz completed [0.00k]
[HOOK - conan-center.py] pre_source(): [IMMUTABLE SOURCES (KB-H010)] OK
openssl/1.1.1s: Configuring sources in C:\Users\spaceim\.conan\data\openssl\1.1.1s\_\_\source\src
Downloading openssl-1.1.1s.tar.gz completed [9637.68k] openssl/1.1.1s: enssl/1.1.1s:
openssl/1.1.1s:
[HOOK - conan-center.py] post_source(): [LIBCXX MANAGEMENT (KB-H011)] OK
[HOOK - conan-center.py] post_source(): [CPPSTD MANAGEMENT (KB-H022)] OK
[HOOK - conan-center.py] post_source(): [SHORT_PATHS USAGE (KB-H066)] OK
openssl/1.1.1s: Copying sources to build folder
openssl/1.1.1s: Building your package in C:\Users\spaceim\.conan\data\openssl\1.1.1s\_\_\build\bb11550f8236e219d08dd4d948c91d66592f7318
openssl/1.1.1s: Generator txt created conanbuildinfo.txt
openssl/1.1.1s: Calling generate()
openssl/1.1.1s: Aggregating env generators
[HOOK - conan-center.py] pre_build(): [FPIC MANAGEMENT (KB-H007)] 'fPIC' option not found
[HOOK - conan-center.py] pre_build(): [FPIC MANAGEMENT (KB-H007)] OK
openssl/1.1.1s: Calling build()
openssl/1.1.1s: Apply patch (portability): TVOS and WatchOS don't like fork()
openssl/1.1.1s: gen_info = {'CFLAGS': ['-m64', '-O3'], 'CXXFLAGS': ['-m64', '-O3'], 'DEFINES': ['NDEBUG'], 'LDFLAGS': ['-m64']}
openssl/1.1.1s: using target: mingw-conan-Release-Windows-x86_64-gcc-12 -> mingw64
openssl/1.1.1s: my %targets = (
"mingw-conan-Release-Windows-x86_64-gcc-12" => {
inherit_from => [ "mingw64" ],
cflags => add("-m64 -O3"),
cxxflags => add("-m64 -O3"),
defines => add(["NDEBUG"]),
includes => add(),
lflags => add("-m64"),
cc => "D:/mingw64/bin/gcc.exe",
cxx => "D:/mingw64/bin/g++.exe",
},
);
openssl/1.1.1s: activated option: shared
openssl/1.1.1s: activated option: shared
openssl/1.1.1s: ['"mingw-conan-Release-Windows-x86_64-gcc-12"', 'shared', '--prefix=/', '--openssldir="res"', 'no-unit-test', 'threads', 'PERL=C:\\Users\\spaceim\\.conan\\data\\strawberryperl\\5.30.0.1\\_\\_\\package\\ca33edce272a279b24f87dc0d4cf5bbdcffbc187\\bin\\perl.exe', 'no-tests', '--release', 'no-md2', 'shared']
Configuring OpenSSL version 1.1.1s (0x1010113fL) for mingw-conan-Release-Windows-x86_64-gcc-12
Using os-specific seed configuration
******************************************************************************
This perl implementation doesn't produce Unix like paths (with forward slash
directory separators). Please use an implementation that matches your
building platform.
This Perl version: 5.30.0 for MSWin32-x64-multi-thread
******************************************************************************
openssl/1.1.1s:
openssl/1.1.1s: ERROR: Package 'bb11550f8236e219d08dd4d948c91d66592f7318' build failed
openssl/1.1.1s: WARN: Build folder C:\Users\spaceim\.conan\data\openssl\1.1.1s\_\_\build\bb11550f8236e219d08dd4d948c91d66592f7318\build-release
ERROR: openssl/1.1.1s: Error in build() method, line 724
self.run(f'{self._perl} ./Configure {args}')
ConanException: Error 255 while executing C:\Users\spaceim\.conan\data\strawberryperl\5.30.0.1\_\_\package\ca33edce272a279b24f87dc0d4cf5bbdcffbc187\bin\perl.exe ./Configure "mingw-conan-Release-Windows-x86_64-gcc-12" shared --prefix=/ --openssldir="res" no-unit-test threads PERL=C:\Users\spaceim\.conan\data\strawberryperl\5.30.0.1\_\_\package\ca33edce272a279b24f87dc0d4cf5bbdcffbc187\bin\perl.exe no-tests --release no-md2 shared
There was a problem hiding this comment.
Hi @SpaceIm,
Sorry for breaking this. As you note, I had misunderstood the Conan 2.0 adoption of win_bash. Thanks for fixing it.
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Specify library name and version: openssl/1.x.x
Updated conanfile.py and test_package/conanfile.py to work with Conan 2.0.0-beta4 (Resolves #13991)
Changes involve leveraging Conan 2.0 generators and other syntactic changes
Tested on Linux, Windows, and Mac for 1.0.2u, 1.1.0l, and 1.1.1s