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

openssl: ensure install_name_tool works on MacOS #5308 #9540

Closed
wants to merge 3 commits into from

Conversation

rockdreamer
Copy link
Contributor

Specify library name and version: openssl/1.1.1k

Adds a test to validate that users can effectively run install_name_tool on the generated library when shared=True on MacOS


  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the
    conan-center hook activated.

@SpaceIm
Copy link
Contributor

SpaceIm commented Feb 28, 2022

Not sure that it's the responsability of conan-center recipe to ckech that.

@rockdreamer
Copy link
Contributor Author

Not sure that it's the responsability of conan-center recipe to ckech that.

The issue I reported (#5308) makes the conan-center-index recipe unusable for my workplace.

In ccdc-opensource/conan-openssl, you can see the workaround I used:

        lflags = env_build.link_flags
        if self.settings.os == "Macos":
            lflags.append("-headerpad_max_install_names")

I want to upstream the change but it doesn't make much sense on its own if it can't be tested.

So I've pushed a test first to see if it fails. If it does, I plan on pushing the change above that fixes the test, otherwise I'll keep the workaround recipe in the company org.

We can also have a wider discussion in a separate conan issue: should all dylibs built by conan recipes be checked in the same way? Not being able to change rpaths is a rather limiting defect, it means I can't distribute conan-built libraries that exhibit this behaviour and must rely on DYLD_LIBRARY_PATH. A really bad idea on modern macos systems.

@conan-center-bot

This comment has been minimized.

@SpaceIm
Copy link
Contributor

SpaceIm commented Feb 28, 2022

Not sure that it's the responsability of conan-center recipe to ckech that.

The issue I reported (#5308) makes the conan-center-index recipe unusable for my workplace.

In ccdc-opensource/conan-openssl, you can see the workaround I used:

        lflags = env_build.link_flags
        if self.settings.os == "Macos":
            lflags.append("-headerpad_max_install_names")

I want to upstream the change but it doesn't make much sense on its own if it can't be tested.

So I've pushed a test first to see if it fails. If it does, I plan on pushing the change above that fixes the test, otherwise I'll keep the workaround recipe in the company org.

We can also have a wider discussion in a separate conan issue: should all dylibs built by conan recipes be checked in the same way? Not being able to change rpaths is a rather limiting defect, it means I can't distribute conan-built libraries that exhibit this behaviour and must rely on DYLD_LIBRARY_PATH. A really bad idea on modern macos systems.

What don't you add -headerpad_max_install_names to LDFLAGS of your profile and build from source. I'm not sure that's the task of conan-center. I'm quite sure that this flag has nothing to do in build system files of a library (I would be surprised than openssl accepts to hardcode this flag), it's a custom flag to inject externally.
It's (or was) a default flag in legacy homebrew: Homebrew/legacy-homebrew#20233. But Homebrew is not a language package manager, so I don't know whether it's a good idea to follow the same policy.

closes conan-io#5308
This makes it possible to change rpaths of the generated libraries
@rockdreamer
Copy link
Contributor Author

What don't you add -headerpad_max_install_names to LDFLAGS of your profile and build from source. I 'm not sure that's the task of conan-center.

I hope you'll excuse me for desiring a more authoritative answer than that :)

The way I see this is that I've found a shortcoming with the product of a recipe, provided a test case that is failing and now adding a patch to hopefully fix the problem. Unless a conan developer steps in and declares/documents the products of conan-center-index as not fit for reuse, I don't see why making them usable for what I consider a valid case is 'not the task of conan-center'. By adding the test before the fix, I'm trying to make sure the issue exists in the recipe and not in my infrastructure/builds and reduce the administrative burden for the conan team and volunteers as much as possible.

If there's an alternate take on what conan and conan-center is, I'd like to know about it because I've invested a lot of time in and with it already!

@SpaceIm
Copy link
Contributor

SpaceIm commented Feb 28, 2022

For sure, this flag is important to be able to set custom RPATH afterwards if you want to relocate shared libs outside of conan tree. But if this flag is added to openssl recipe, it should be added to all recipes, so it would place a huge burden on conan-center.

@rockdreamer
Copy link
Contributor Author

For sure, this flag is important to be able to set custom RPATH afterwards if you want to relocate shared libs outside of conan tree. But if this flag is added to openssl recipe, it should be added to all recipes, so it would place a huge burden on conan-center.

I totally understand the will to have consistency and the dread at the thought of checking all packages in conan-center index! 🤯

However, openssl is the only inconsistent one for me! It is the only recipe where setting a custom rpath afterwards fails for me out of about 70-ish packages we use. I think this is caused by some little difference in the openssl build system.

Hence why I'm purposefully proposing a recipe change and not a generic solution. Sometimes a local change can help create consistency!
If this change helps surface the need for rpath-alterable recipes, all the better! That can then be addressed separately, prioritised, candidate solutions for solving this generically proposed.

@conan-center-bot

This comment has been minimized.

@rockdreamer
Copy link
Contributor Author

A look at the logs shows that this breaks cross-compilation. The build is trying to create test executables in x86_64 mode

@conan-center-bot
Copy link
Collaborator

Failure in build 3 (66a5fda3ecef02c3a404217599049f1f478915c3):

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

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

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

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

  • openssl/1.1.1d@:
    Didn't run or was cancelled before finishing

  • openssl/1.1.0k@:
    CI failed to create some packages (All logs)

    Logs for packageID 3558c54d13382045caaf9174821b7c2ba01fc8bc:
    [settings]
    arch=armv8
    build_type=Debug
    compiler=apple-clang
    compiler.libcxx=libc++
    compiler.version=13.0
    os=Macos
    [options]
    openssl:shared=False
    
    [...]
            return BIO_printf(out, "%"BIO_PRI64"d\n", **(int64_t **)pval);
                                    ~~~~~~~~~~~~~     ^~~~~~~~~~~~~~~~~~
                                    %lld
    crypto/asn1/x_int64.c:106:47: warning: format specifies type 'unsigned long' but the argument has type 'uint64_t' (aka 'unsigned long long') [-Wformat]
        return BIO_printf(out, "%"BIO_PRI64"u\n", **(uint64_t **)pval);
                                ~~~~~~~~~~~~~     ^~~~~~~~~~~~~~~~~~~
                                %llu
    2 warnings generated.
    ar: creating archive libssl.a
    apps/enc.c:554:63: warning: format specifies type 'unsigned long' but the argument has type 'uint64_t' (aka 'unsigned long long') [-Wformat]
            BIO_printf(bio_err, "bytes read   :%8"BIO_PRI64"u\n", BIO_number_read(in));
                                               ~~~~~~~~~~~~~~     ^~~~~~~~~~~~~~~~~~~
                                               %8llu
    apps/enc.c:555:63: warning: format specifies type 'unsigned long' but the argument has type 'uint64_t' (aka 'unsigned long long') [-Wformat]
            BIO_printf(bio_err, "bytes written:%8"BIO_PRI64"u\n", BIO_number_written(out));
                                               ~~~~~~~~~~~~~~     ^~~~~~~~~~~~~~~~~~~~~~~
                                               %8llu
    2 warnings generated.
    apps/s_cb.c:1006:35: warning: format specifies type 'unsigned long' but the argument has type 'uint64_t' (aka 'unsigned long long') [-Wformat]
                       opt_getprog(), (uint64_t)len);
                                      ^~~~~~~~~~~~~
    apps/s_client.c:2631:20: warning: format specifies type 'unsigned long' but the argument has type 'uint64_t' (aka 'unsigned long long') [-Wformat]
                       BIO_number_read(SSL_get_rbio(s)),
                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    apps/s_client.c:2632:20: warning: format specifies type 'unsigned long' but the argument has type 'uint64_t' (aka 'unsigned long long') [-Wformat]
                       BIO_number_written(SSL_get_wbio(s)));
                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    1 warning generated.
    2 warnings generated.
    ld: warning: ignoring file test/rsa_complex.o, building for macOS-x86_64 but attempting to link with file built for unknown-arm64
    ar: creating archive libcrypto.a
    Undefined symbols for architecture x86_64:
      "_main", referenced from:
         implicit entry/start for main executable
    ld: symbol(s) not found for architecture x86_64
    clang: error: linker command failed with exit code 1 (use -v to see invocation)
    make[2]: *** [link_app.] Error 1
    make[1]: *** [test/rsa_complex] Error 2
    make[1]: *** Waiting for unfinished jobs....
    make: *** [all] Error 2
    openssl/1.1.0k: WARN: Build folder is dirty, removing it: /Users/jenkins/w/BuildSingleReference@4/.conan/data/openssl/1.1.0k/_/_/build/3558c54d13382045caaf9174821b7c2ba01fc8bc
    openssl/1.1.0k: ERROR: Package '3558c54d13382045caaf9174821b7c2ba01fc8bc' build failed
    openssl/1.1.0k: WARN: Build folder /Users/jenkins/w/BuildSingleReference@4/.conan/data/openssl/1.1.0k/_/_/build/3558c54d13382045caaf9174821b7c2ba01fc8bc
    ERROR: openssl/1.1.0k: Error in build() method, line 776
    	self._make()
    while calling '_make', line 716
    	self._run_make()
    while calling '_run_make', line 671
    	self.run(" ".join(command), win_bash=self._win_bash)
    	ConanException: Error 2 while executing /usr/bin/make -j4
    
  • openssl/1.1.1i@:
    Didn't run or was cancelled before finishing

  • openssl/1.1.1k@:
    Didn't run or was cancelled before finishing

  • openssl/1.1.1h@:
    Didn't run or was cancelled before finishing

  • openssl/1.1.1c@:
    Didn't run or was cancelled before finishing

  • openssl/1.1.1e@:
    Didn't run or was cancelled before finishing

  • openssl/1.1.0l@:
    Didn't run or was cancelled before finishing

  • openssl/1.1.1g@:
    Didn't run or was cancelled before finishing

  • openssl/1.0.2s@:
    Didn't run or was cancelled before finishing

  • openssl/1.0.2t@:
    Didn't run or was cancelled before finishing

  • openssl/1.0.2u@:
    Didn't run or was cancelled before finishing


Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability.

@rockdreamer rockdreamer closed this Mar 3, 2022
@SpaceIm
Copy link
Contributor

SpaceIm commented Oct 31, 2022

It seems that boost also suffers of this issue in shared libs on macOS: #13892. Actually I think that most conan-center recipes are OK (those relying on CMake, Meson & Autotools?), but there are few ones with more exotic build systems where this flag should indeed be injected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants