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

[PkgConfigDeps] Added bindir to generated .pc files #13623

Merged
merged 9 commits into from
Jun 14, 2023

Conversation

AbrilRBS
Copy link
Member

@AbrilRBS AbrilRBS commented Apr 5, 2023

Changelog: Feature: Added bindir to generated .pc file in PkgConfigDeps.
Changelog: Bugfix: Removed libdir1 and includedir1 as the default index. Now, PkgConfigDeps creates the libdir and includedir variables by default in .pc files.
Docs: conan-io/docs#3269

After asking @bruchar1, this seems to be the mvp for the asked feature :)

Closes #13532

UPDATED:

In a nutshell:

  • I think this bindir is harmless.
  • Honoring libdir, includedir and bindir as default variables created (if several ones have to be created, then, they'll be libdir, libdir1, libdir2, and so on). After running a grep in CCI, these libraries could be affected by this change (they'd need a migration if they use this latest feature):
$ ag "dir1"
gdk-pixbuf/all/conanfile.py
202:            "gdk_pixbuf_binarydir": "${libdir1}/gdk-pixbuf-2.0/2.10",

glib/all/conanfile.py
263:            # Can't use libdir here as it is libdir1 when using the PkgConfigDeps generator.

gstreamer/all/conanfile.py
122:            # PkgConfigDep uses libdir1 instead of libdir, so the path is spelled out explicitly here.

So gdk-pixbuf, glib and gstreamer should be fixed a priori.

  • We have functional tests running the meson templates (lib and exe) in all platforms, so I think it's enough to see those tests passing without any errors.

@AbrilRBS AbrilRBS marked this pull request as ready for review April 14, 2023 12:46
@AbrilRBS AbrilRBS requested a review from memsharded April 14, 2023 12:46
@AbrilRBS AbrilRBS self-assigned this May 4, 2023
@AbrilRBS
Copy link
Member Author

AbrilRBS commented May 4, 2023

Hi @bruchar1 sorry about the delay getting this merged.
@franramirez688 and I want to provide tests for this, but we currently lack a knowledge of the use-case for this feature (i.e, where would this be useful?) I'd appreciate if you could provide me with a repro case of where this comes into play so that I can better understand it :)

@AbrilRBS AbrilRBS requested a review from franramirez688 May 4, 2023 10:39
@bruchar1
Copy link
Contributor

bruchar1 commented May 8, 2023

Hello.

I use conan with meson build system on Windows. meson uses pkgconfig generated files to find libraries provided by conan. To be able to run tests on Windows, meson must add paths to DLLs in the PATH environment variable. However, the pkgconfig file currently only provide paths to .lib link libraries, and there is no way to know for sure the path to the DLL. Once conan provide this path, I will be able to use it in meson.

This is kind of chicken and egg problem, because meson will not try to use variables that do not exist in the pkgconfig file, but conan cannot test it with meson before meson actually uses the variable...
The change I did to read that variable is in https://github.com/mesonbuild/meson/pull/11649/files#diff-49a709b5667321b5e8f9735b43b56bcc1714b13532f01884a69e31baa7418abeR1079 . However, it does not handle the suffix to the variable name, because meson developers wanted a real use case before adding this feature (mesonbuild/meson#11584 (comment))

@AbrilRBS
Copy link
Member Author

AbrilRBS commented May 9, 2023

Thanks for the insight, I'll get back to you with my progress in a few days :)

@franramirez688
Copy link
Contributor

franramirez688 commented May 17, 2023

Hi, @bruchar1 - Thanks for your comment, and sorry for the delay! 😅

Could you provide a little repository to see your use case and why Conan's current behavior is insufficient?

For instance, if you run:

$ conan new meson_lib -d name=hello -d version=1.0
$ conan create  . -o shared=True

The test_package uses pkg-config, and the DLL path is saved in libdir, so what am I missing? I imagine that your use case is more complex, doesn't it?

@bruchar1
Copy link
Contributor

Unfortunately, I cannot provide a repo for this.

The detail you are missing is that it's not the DLL path that is in libdir, but the import lib path used for linking with the DLL. That's fine for compiling and linking. What I am missing is a way to run tests using that lib, without installing it.

Suppose you have an executable target, depending on a shared library provided by conan. On Windows, the way the exe can find the DLL is to have the DLL in the PATH env var. However, to link to the DLL, only the .lib import library is needed. The generated .pc file generated from conan provides the path to the .lib (usually in the lib folder), but not to the .dll (usually in the dll folder). If the executable is a test program that I want to run using meson test, meson will add the paths to all the required libraries to the PATH env var, to ensure the test executable find all DLLs. However, it actually doesn't have a reliable way to find the path to the DLLs, apart from using the heuristic that DLLs are usually located in in a sibling directory called bin.

@memsharded
Copy link
Member

Hi @bruchar1

The responsible for the runtime execution location of shared libs is the VirtualRunEnv generator, that runs by default in Conan2.0. Any self.run(cmd, env="conanrun") would have the run environment activated, that will put the necessary DLLs from dependencies in the PATH, or the necessary .so or .dylib in the LD_LIBRARY_PATH or DYLD_LIBRARY_PATH. Maybe the issue is that meson.test() is internally not activating the "conanrun" environment?

Maybe this PR was not addressing the original issue, as it might not be possible at all via PkgConfig?

@bruchar1
Copy link
Contributor

Maybe the issue is that meson.test() is internally not activating the "conanrun" environment?

That would probably work if I was invoking meson from conan. But the way I use it is to use conan to generate pkgconfig files and meson toolchain file, and then to invoke meson directly. This is needed to avoid the overhead of calling meson as a subprocess, and to allow the flexibility of using meson features directly. Therefore, in this context, meson know nothings about conan.

Furthermore, bindir is a standard installation variable, as seen in https://www.gnu.org/prep/standards/html_node/Directory-Variables.html.

Maybe this PR was not addressing the original issue, as it might not be possible at all via PkgConfig?

I don't think so. Exposing bindir via a variable is the way to go, imho.

The only part that tricks me is the numbered suffix. Since in most cases, there should only be one bindir, having bindir instead of bindir1 would be more conventional. Maybe could bindir be the same as bindir1? Or maybe could it be bindir, bindir2, bindir3... ?

@memsharded
Copy link
Member

Even if not calling meson from conan, the environment can be activated before calling meson.

So I understand from your explanation that Meson has some feature that is able to automatically set the PATH environment variable while executing tests based on the information gathered from .pc files of the dependencies?

Furthermore, bindir is a standard installation variable, as seen in https://www.gnu.org/prep/standards/html_node/Directory-Variables.html.

Not sure how it works in a Windows machine, not following GNU conventions. The dependencies packages will contain a bin subfolder with the DLLs, still I am curious how Meson will leverage that to run its tests.

Do you have some link to Meson docs or example that describes this it that is the case? I am afraid that I dont know enough about Meson to really understand the issue here.

@bruchar1
Copy link
Contributor

Even if not calling meson from conan, the environment can be activated before calling meson.

I see... Yes it could work. The only drawback I see is that it would put all conan dependencies path into PATH, and not only the ones I need for a specific test executable (because I use one conanfile for a huge monorepo with severals libs and tests).

So I understand from your explanation that Meson has some feature that is able to automatically set the PATH environment variable while executing tests based on the information gathered from .pc files of the dependencies?

On Linux, meson use rpaths for that. On Windows, it use a big hack to build the PATH variable. It works well for dependencies defined inside the project, but for external dependencies (like pkgconfig dependencies), it was using the libpath, which is wrong, as I explained before.

The relevant code is here: https://github.com/mesonbuild/meson/pull/11649/files

@CLAassistant
Copy link

CLAassistant commented May 18, 2023

CLA assistant check
All committers have signed the CLA.

@franramirez688 franramirez688 changed the title Add bindir to generated .pc file in PkgConfigDeps [PkgConfigDeps] Added bindir to generated .pc files Jun 1, 2023
@franramirez688 franramirez688 added this to the 2.0.7 milestone Jun 1, 2023
@franramirez688 franramirez688 requested a review from jcar87 June 1, 2023 15:06
Copy link
Member Author

@AbrilRBS AbrilRBS left a comment

Choose a reason for hiding this comment

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

Can't approve my PR, but this now looks good, thanks a lot @franramirez688 for the work and @bruchar1 for your patience :)

@db4
Copy link
Contributor

db4 commented Feb 17, 2024

Can this be backported to Conan 1.x? Some libraries use pkg-config as a configuration tool (qt is an example that I came across myself). It requires libdir and doesn't accept libdir1. Due to that the current CCI qt recipe cannot be built with gstreamer support for Conan 1.x. This will likely affect other CCI recipes as well.

I've tried to backport it locally leaving alone all testing infrastructure - cherry-picked 69d3b27 seems to work.

@AbrilRBS AbrilRBS deleted the rr/pkg-conf-bindir branch February 17, 2024 23:17
@franramirez688
Copy link
Contributor

Can this be backported to Conan 1.x? Some libraries use pkg-config as a configuration tool (qt is an example that I came across myself). It requires libdir and doesn't accept libdir1. Due to that the current CCI qt recipe cannot be built with gstreamer support for Conan 1.x. This will likely affect other CCI recipes as well.

I've tried to backport it locally leaving alone all testing infrastructure - cherry-picked 69d3b27 seems to work.

Hi @db4
I'm not sure that it could be done. This was a breaking change, and that's why we introduced it in Conan 2.x. Anyway, let me have a look at the recipe because I'm sure that we could add a temporary workaround.

@db4
Copy link
Contributor

db4 commented Feb 18, 2024

@franramirez688 Why do you think that replacing libdir1 with standard libdir is a breaking change? What known tool/recipe depends on the former? BTW this problem was originally reported for Conan 1 in #12279, but was never fixed for it.

As for qt recipe - I don't think it would be easy to reproduce. My understanding is that gstreamer support in qt is currently badly broken, I had to use not yet merged V2-compatible gst-plugins-base recipe and many private fixes to make the whole thing compile.

@franramirez688
Copy link
Contributor

@franramirez688 Why do you think that replacing libdir1 with standard libdir is a breaking change? What known tool/recipe depends on the former? BTW this problem was originally reported for Conan 1 in #12279, but was never fixed for it.

It's breaking because some recipes use the pkg_config_custom_content to add some additional content and they could be using libdir1 as the default one. So, yes, it's a breaking change for Conan 1.x consumers. Let's not only think about recipes in Conan Center but also users who could have their recipes locally. Have a look at this comment conan-io/conan-center-index#21137 (review) related to this

@Talkless
Copy link

Can this be backported to Conan 1.x?

I'm still using 1.x, and I can't build gst-plugins-base master branch, as it requires Meson >= 1.1, and it complains that libdir is not available:

Found pkg-config: /home/vincas/.conan/data/pkgconf/1.9.3/_/_/package/24647d9fe8ec489125dfbae4b3ebefaf7581674c/bin/pkgconf (1.9.3)
Found CMake: /usr/bin/cmake (3.25.1)
Run-time dependency libdrm found: NO (tried pkgconfig and cmake)
Run-time dependency x11 found: YES 1.7.2
Run-time dependency gio-2.0 found: YES 2.74.0
Run-time dependency gio-unix-2.0 found: YES 2.74.0
Run-time dependency gmodule-no-export-2.0 found: YES 2.74.0
Dependency gdk-pixbuf-2.0 skipped: feature examples disabled
Dependency gtk+-3.0 skipped: feature examples disabled
Dependency gtk+-x11-3.0 skipped: feature examples disabled
WARNING: pkgconfig variable 'libdir' not defined for dependency gio-2.0.

(gio-2.0.pc file has libdir1')

And Meson 1.3.1 shows even stranger error:

Run-time dependency gio-2.0 found: YES 2.74.0
Run-time dependency gio-unix-2.0 found: YES 2.74.0
Run-time dependency gmodule-no-export-2.0 found: YES 2.74.0
Dependency gdk-pixbuf-2.0 skipped: feature examples disabled
Dependency gtk+-3.0 skipped: feature examples disabled
Dependency gtk+-x11-3.0 skipped: feature examples disabled

../src/meson.build:346:16: ERROR: Could not get pkg-config variable and no default provided for <PkgConfigDependency gio-2.0: True ['>= 2.64.0']>

Are there known workarounds I could try?

@db4
Copy link
Contributor

db4 commented Feb 20, 2024

@franramirez688 I still believe that fixing PkgConfigDeps in Conan 1.x is a way to go. PkgConfigDeps was always advertised as an experimental Conan 2 feature:

This feature is still under development, while it is recommended and usable and we will try not to break them in future releases, some breaking changes might still happen if necessary to prepare for the Conan 2.0 release.

Many CCI recipes depend or will depend on Conan 2's PkgConfigDeps behavior, and that will generate troubles for users who still have to stick with Conan 1.x for various reasons. If you're strongly against direct fix, maybe conditional Conan 2-compatible behavior is possible? (switched on via some setting).

@franramirez688
Copy link
Contributor

franramirez688 commented Feb 20, 2024

Many CCI recipes depend or will depend on Conan 2's PkgConfigDeps behavior, and that will generate troubles for users who still have to stick with Conan 1.x for various reasons. If you're strongly against direct fix, maybe conditional Conan 2-compatible behavior is possible? (switched on via some setting).

@db4 Ok, I will discuss this issue with the team. I'll tell you when we address it. Thanks for the comment!

Are there known workarounds I could try?

@Talkless I think you should open a new issue in the repo https://github.com/conan-io/conan-center-index/issues because your comment seems related to the recipe.

@jcar87
Copy link
Contributor

jcar87 commented Feb 20, 2024

Hi @db4 - could you provide a specific example of a recipe in conan-center-index and a workflow that does not work because of this? @Talkless , thank you for providing an example, we will look into this.

The variable names at the top of the .pc files (the ones with the = sign and an assignment) are not part of any standard specification. The documentation specifically describes the variables as freeform: they can have any name. It is the keywords that have set names. So long as the Libs references the right variables, it should not matter what the variable was called.

Some recipes in Conan Center used to make an assumption on the variable names atop the .pc files - which is wrong and should be fixed in the recipes themselves.

It would be good for us to understand which other tools are making assumptions about the contents of the .pc files in a way that breaks encapsulation - we did find such an example with Meson, but we want to understand how generalised this is.

@db4
Copy link
Contributor

db4 commented Feb 20, 2024

@jcar87

could you provide a specific example of a recipe in conan-center-index and a workflow that does not work because of this?

qt with with_gstreamer=True. But you can't easily reproduce it - qt requires gst-plugins-base that is also broken (as @Talkless explained above). I've used not-yet-merged gst-plugins-base Conan V2-compatible recipe with some private fixes.

@jcar87
Copy link
Contributor

jcar87 commented Feb 20, 2024

@jcar87

could you provide a specific example of a recipe in conan-center-index and a workflow that does not work because of this?

qt with with_gstreamer=True. But you can't easily reproduce it - qt requires gst-plugins-base that is also broken (as @Talkless explained above). I've used not-yet-merged gst-plugins-base Conan V2-compatible recipe with some private fixes.

are you able to paste a log with the specific failures that you are getting? Please open a new issue about it so that we can track this

@db4
Copy link
Contributor

db4 commented Feb 20, 2024

Qt build log also doesn't tell you very much: just that GStreamer is not found (Qt runs pkg-config with --silence-errors and doesn't explain what's wrong with .pc files). I've spent enough time to guess what's going on and prove it with locally-patched Conan. But OK, I'll open a new issue for that. Should it be conan-center-index issue for qt recipe?

@jcar87
Copy link
Contributor

jcar87 commented Feb 20, 2024

Qt build log also doesn't tell you very much: just that GStreamer is not found (Qt runs pkg-config with --silence-errors and doesn't explain what's wrong with .pc files). I've spent enough time to guess what's going on and prove it with locally-patched Conan. But OK, I'll open a new issue for that. Should it be conan-center-index issue for qt recipe?

yes please, that would be great! We can try and replicate and trace the build logs of Qt.

It's interesting that you mention that qt runs the pkg-config tool - if this tool is run, then the more reason to believe the variable name (libdir1 vs libdir) is irrelevant: most invocations to pkg-config will be with --cflags, --libs, etc - in which case the variable names are irrelevant. In order for this to be an issue, we'd need a tool that is either reading the variables directly from the .pc file (as we have discovered with Meson) or running pkg-config with --variable and expecting a specific variable name to exist (which is not guaranteed as they can be arbitrary).

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

Successfully merging this pull request may close these issues.

[feature] Add bindirs to pkgconfig generator
8 participants