fix: pass pkg-config (extra) args when evaluating the expression

[MisterDA]

pkgconf uses the personality to locate the correct library when the library can be found with multiple toolchains. For instance, on Cygwin, pkgconf could detect libcurl (built with cygwin1.dll), licurl (built with x86_64-w64-mingw32), or libcurl (built with i686-w64-mingw32). It needs the right --personality flag to select between these libraries, which we pass when asking for the --cflags or --libs. We also need it when validating the expression, or pkgconf returns an error:

$ (cd _build/default && config/discover.exe)
which: pkgconf
-> found: C:\Users\Antonin\AppData\Local\opam\default-2\bin\pkgconf.exe
run: C:\Users\Antonin\AppData\Local\opam\default-2\bin\pkgconf.exe --print-errors "libcurl >= 7.28.0"
-> process exited with code 1
-> stdout:
-> stderr:
| Package libcurl was not found in the pkg-config search path.
| Perhaps you should add the directory containing `libcurl.pc'
| to the PKG_CONFIG_PATH environment variable
| Package 'libcurl' not found
Error: Package libcurl was not found in the pkg-config search path.
Perhaps you should add the directory containing `libcurl.pc'
to the PKG_CONFIG_PATH environment variable
Package 'libcurl' not found
$ ~/AppData/Local/opam/default-2/bin/pkgconf.exe --personality x86_64-w64-mingw32 --print-errors "libcurl >= 7.28.0

This is a nasty bug for me, as there's no way of passing the personality flag to pkgconf outsite of Dune: the PKG_CONFIG_ARGN is just exposed by Dune and CMake and isn't actually recognized by pkgconf.

cf #10937

[maiste]

(Once the CI passes the test 😄)

[Leonidas-from-XIV]

It's most likely ok, I'd just want to understand where the --personality change comes from.

[Leonidas-from-XIV]

Just so I understand the issue: we always had the --personality $TARGET flag in t.pkg_config_args (which I guess is partly populated from PKG_CONFIG_ARGN), we just forgot to pass it to pkgconf?

[MisterDA]

Say we're looking for libcurl. Then, C.Pkg_config.query_expr_err pc ~package:"libcurl" ~expr:"libcurl >= 7.28.0" will do three invocations:

$ pkgconf "libcurl >= 7.28.0"
$ pkgconf $PKG_CONFIG_ARGN --libs libcurl
-lcurl
$ pkgconf $PKG_CONFIG_ARGN --cflags libcurl

I just found out that if we're not passing $PKG_CONFIG_ARGN to the first invocation, then it might fail because pkgconf would be looking for libcurl in the "default" personality, which may not be the one the current ocaml install is using. So, yes, we forgot to pass it to the first invocation.

Note that PKG_CONFIG_ARG isn't a variable that's understood by pkgconf or pkg-config, it's an escape hatch from the build systems (CMake and Dune) to allow users to pass extra variables to pkgconf/pkg-config invocations—provided the build system doesn't forget to pass the variable!

[Leonidas-from-XIV]

I get that, but from what I understand there were two bugs being fixed in this PR:

1. PKG_CONFIG_ARGN wasn't respected when doing the call (thus -static wasn't passed in the test) and now it is. That much makes sense to me.
2. Somehow --personality was not passed either and now it is. It did not come from PKG_CONFIG_ARGN (since the first dune build invocation in the test does not have it set) so I assume it was another bug that also existed and is being fixed by this PR.

Am I right in this interpretation?

Please add a changelog entry however since it would be nice for people to learn about this fix when we cut the next Dune release.

[MisterDA]

I think the bug was rather that Dune forgot to pass the personality flag to the first invocation of pkgconf.

Reading about --static:

Compute a deeper dependency graph and use compiler/linker flags intended for static linking.

I don't think it matters much in the first invocation (just to detect whether the package is available).

The mechanism Dune uses to pass extra flags to pkgconf, whether auto-detected, or from the user through PKG_CONFIG_ARGN, works when requesting --libs and --cflags.

Dune might even be overzealous in doing that first invocation. If we don't get rid of it, maybe just passing the personality flag is enough and we don't need to pass the full PKG_CONFIG_ARGN. I'm not sure if that would be surprising to users.

[MisterDA]

Sorry, I wasn't very clear. t.pkg_config_args is either the content of PKG_CONFIG_ARGN if set, or --personality=$target (set by Dune). Either way it wasn't passed to the first invocation. I don't think the first invocation need the --static flag anyway, but the user might be willing to pass additional flags. Thinking about it, I'd be surprised if this first invocation is needed at all, meaning that we can pass the content of the expression to when we look for flags.