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

[pkgconf] pkgconf/1.7.4: Binary does not work in shared build #7540

Closed
daravi opened this issue Oct 5, 2021 · 16 comments · Fixed by #10494
Closed

[pkgconf] pkgconf/1.7.4: Binary does not work in shared build #7540

daravi opened this issue Oct 5, 2021 · 16 comments · Fixed by #10494
Labels
bug Something isn't working

Comments

@daravi
Copy link
Contributor

daravi commented Oct 5, 2021

When building pkgconf with -o *:shared=True I end up with a pkgconf binary that when run gives the following error:

$ /opt/conan/pkgconf/1.7.4/3rdparty/stable/package/fb49f7b339424395408c6d9629736e5998939ecf/bin/pkgconf
/opt/conan/pkgconf/1.7.4/3rdparty/stable/package/fb49f7b339424395408c6d9629736e5998939ecf/bin/pkgconf: symbol lookup error: /opt/conan/pkgconf/1.7.4/3rdparty/stable/package/fb49f7b339424395408c6d9629736e5998939ecf/bin/pkgconf: undefined symbol: pkgconf_cross_personality_default

This seems to be causing #7539.

Package and Environment Details (include every applicable attribute)

  • Package Name/Version: glib/2.69.2
  • Operating System+version: AlmaLinux 8.4
  • Compiler+version: Clang 10.0.1
  • Conan version: 1.40.3
  • Python version: Python 3.9.2

output.log
meson-log.txt

@madebr
Copy link
Contributor

madebr commented Oct 5, 2021

Most, if not all, recipes are designed to run inside a conan run environment.
You can create one as follows:

conan install pkgconf/1.7.4@ -o pkgconf:shared=True -g virtualrunenvironment
source activate_run.sh

# do your thing

source deactivate_run.sh

I guess adding a rpaths to the pkgconf executalbe would be possible.
But the problem remains when it links to an external shared library.

@daravi
Copy link
Contributor Author

daravi commented Oct 5, 2021

@madebr That makes sense. So considering that in #7539 the consumer of pkgconf is not able to run pkgconf (I am assuming for the same reason of not being in run environment), is this a bug in this recipe or the other one? pkgconf should be able to be consumed with shared option in an intermediate recipe correct?

@madebr
Copy link
Contributor

madebr commented Oct 5, 2021

The problem is that conan does not run its build helpers in a run environment.
Conan runs meson here.
As you see, there's no run_environment=True added.
The CMake and AutotoolsBuildEnvironment build helpers have the same issue.

It's possible to fix this by surrounding with a tools.environment_append(tools.RunEnvironment(self).vars) context.
But imho, the real issue is that conan is missing a flag to enable a run environment on build helpers.

I think conan should add a option to the CMake/Meson/AutotoolsBuildEnvironment constructs:
e.g.

meson = Meson(self, run_environment=True)

which should get passed down to every ConanFile.run.

The only problem is that these build helpers will soon get deprecated.
Perhaps a issue at https://github.com/conan-io/conan would be helpful, to make sure they are added to their successors.

@daravi
Copy link
Contributor Author

daravi commented Oct 5, 2021

@madebr I am wondering. If a package is a build_requirement, shouldn't its run_evironment be added to the build environment of the consumer? Sure normally the bin directory should only be added to the path at runtime. But if something is a build_requirement then chances are that the binary would be needed at build time for the consumer. For running tests I agree we need to run them in the run environment. But it seems wrong to me that we enforce run environment when we only need the build requirements to be in the run environment (and not for example the bin directory of a regular requires). Hypothetically say we have protobuf as a requires but want to use the system protoc during build time. And have pkgconf as a build requirement. If we run the build in the run environment we also bring in the unwanted protoc.

@madebr
Copy link
Contributor

madebr commented Oct 5, 2021

To be honest, I'm not a frequent user of virtualenv/virtualbuildenv/virtualrunenv generators.
Adding PATHs of a host requirement is a clear no-no.

I just know that when using self.run(..., run_environment=True, PATH and LD_LIBRARY_PATH are initialized correctly.
This works when cross building + not cross building + also in the test package.
But then again, most test packages have the main recipe as a host requirement.

Let's ask @uilianries / @jgsogo in what way/how we should support shared build requirements.

@SpaceIm
Copy link
Contributor

SpaceIm commented Oct 6, 2021

run_environment is not sufficient on MacOS due to SIP (but it's fine on Linux).

Anyway, as a consumer I always force static in all build requirements and strongy discourage a brutal -o *:shared=True, so something like this with 2 profiles:

conan install <conanfile path> -o:b *:shared=False -o:h *:shared=True

@daravi
Copy link
Contributor Author

daravi commented Oct 6, 2021

@SpaceIm I agree that is better practice. But I would say that should be documented if it is a limitation. And then the shared option should probably be deleted when a recipe is used as a build requirement. If not then I argue that it should work.

@SpaceIm
Copy link
Contributor

SpaceIm commented Oct 6, 2021

And then the shared option should probably be deleted when a recipe is used as a build requirement.

There is no such feature in conan api.

If not then I argue that it should work.

Any recipe with build requirements which have shared options (even transitively) should wrap build and package commands with tools.run_environment(self). Then it should work on windows and linux. No solution for macos.

@daravi
Copy link
Contributor Author

daravi commented Oct 6, 2021

@SpaceIm Wouldn't that add every PATH to build environment, even those from regular requires? Isn't that unwanted? (see #7540 (comment))

@jgsogo
Copy link
Contributor

jgsogo commented Oct 11, 2021

Hi! This is not an easy topic.

Standalone execution

For sure, when we want to run an executable that is linked dynamically to other libraries, we need to add those other libraries to some path so the system is able to find them, this is why running these executables will require to activate some virtual environment (or use rpaths, which is not possible when we are redistributing already built binaries). As suggested above, the general rule is:

conan install pkgconf/1.7.4@ -o pkgconf:shared=True -g virtualenv
source activate.sh

# do your thing with pkgconf

source deactivate.sh

Running these commands all the required shared libraries will be found and it should work in Windows and Linux for any scenario (even if pkgconf is called from inside any other application). In Macos the SIP protection will prevent some environment variables to be available in the internal calls, and shared libraries won't be found. AFAIK, the only possible workaround for Macos would be to use wrappers or shims (conan-io/conan#7324).

Using as build-requires

When it comes to a conanfile.py we need to pay a little bit more attention. For sure, all the above applies, but there is something else to take into account: Conan will populate the environment for the packages in the build context (typically the tools that are used by the build-system, which are the packages listed as build-requires), this environment should be available in the build() and package() methods. However, which packages are listed in the build context can be different if you are using one profile or two profiles.

Now I'll have a look at the concrete pkg_config example

@jgsogo
Copy link
Contributor

jgsogo commented Oct 11, 2021

On my side (Macos), the pkgconf works if built with shared=True:

⇒  conan info pkgconf/1.7.4@ --profile:host=default --profile:build=default "--options:host=*:shared=True" --paths   
pkgconf/1.7.4
    ID: bda713dd3b257827c8d11a06ac9d824038871572
    BuildID: None
    Context: host
    export_folder: /Users/jgsogo/.conan/data/pkgconf/1.7.4/_/_/export
    source_folder: /Users/jgsogo/.conan/data/pkgconf/1.7.4/_/_/source
    build_folder: /Users/jgsogo/.conan/data/pkgconf/1.7.4/_/_/build/bda713dd3b257827c8d11a06ac9d824038871572
    package_folder: /Users/jgsogo/.conan/data/pkgconf/1.7.4/_/_/package/bda713dd3b257827c8d11a06ac9d824038871572
    Remote: None
    URL: https://github.com/conan-io/conan-center-index
    Homepage: https://git.sr.ht/~kaniini/pkgconf
    License: ISC
    Description: package compiler and linker metadata toolkit
    Topics: conan, pkgconf
    Provides: pkgconf
    Recipe: Cache
    Revision: 5b7befdb48cd709d97c139a9ff3f4101
    Package revision: 2f5ef2674a6ad46fdd2f9f0a1316b3c9
    Binary: Cache
    Binary remote: None
    Creation date: 2021-10-11 11:30:51 UTC

⇒  /Users/jgsogo/.conan/data/pkgconf/1.7.4/_/_/package/bda713dd3b257827c8d11a06ac9d824038871572/bin/pkgconf 
Please specify at least one package name on the command line.

Maybe I'm missing something from your logs. Am I reproducing your scenario?

If it was about the test_package, I created a PR here (#7645) to fix it, pkgconf is also a build-requirement for that test, not just a regular requirement.

@daravi
Copy link
Contributor Author

daravi commented Oct 11, 2021

@jgsogo The issue started with #7539 when I tried to build glib as shared with a single profile, and now I am trying to determine where lies the issue?

  • Should pkgconf recipe be modified?
  • Should glib recipe be modified to add run environment to build context? (When using a single profile) If so wouldn’t that add regular requirements to build context as well?
  • Should conan be patched to add build requirement paths to build context even when using a single profile?
  • Is this a limitation of single profile which means if I need to build glib as shared I have to use two profiles?

@jgsogo
Copy link
Contributor

jgsogo commented Oct 11, 2021

I would suggest always try with two-profiles first and then with one profile. Two-profiles will be the default (and only way) for Conan v2. Using single profile will be deprecated in the future and we cannot make any change to current behavior in Conan v1 because it might break other consumers (even if it looks like a bug, we cannot add/remove any environment variable).

@daravi
Copy link
Contributor Author

daravi commented Oct 12, 2021

@jgsogo So to confirm we don't want to change the behavior of single profile and we don't want to add run environment to build step because that would add all requirement paths to build environment (and not just build requirements). i.e. we don't want to add lines like:

meson = Meson(self, run_environment=True)

in build as suggested in this thread. Correct?

@jgsogo
Copy link
Contributor

jgsogo commented Oct 13, 2021

Changing how the single profile works can be breaking, so it is very unlikely that any modification can be done there. About adding a run_environment=True to Meson constructor, as it would be an opt-in it won't break any existing behavior, so it is something that can be considered. Anyway, this is something related to Conan client development, so better ask in its repository for a final answer.

In this thread, we can talk about existing behavior and recipes, in order to improve them if needed. TBH, I still don't know what is the issue here and what fails on your side.

@ericLemanissier
Copy link
Contributor

this will be fixed by #10494 (by default, it packages only the executable, which statically links to the library)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants