Skip to content

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

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.

Sign up for GitHub

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

Merged
Leonidas-from-XIV merged 1 commit into from
Apr 10, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions doc/changes/11619.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
- fix: pass pkg-config (extra) args in all pkgconfig invocations. A missing --personality flag would result in pkgconf not finding libraries in some contexts. (#11619, @MisterDA)
2 changes: 1 addition & 1 deletion otherlibs/configurator/src/v1.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -735,7 +735,7 @@ module Pkg_config = struct
in
let pc_flags = "--print-errors" in
let { Process.exit_code; stderr; _ } =
Process.run_process c ~dir ?env t.pkg_config [ pc_flags; expr ]
Process.run_process c ~dir ?env t.pkg_config (t.pkg_config_args @ [ pc_flags; expr ])
in
if exit_code = 0
then (
Expand Down
7 changes: 5 additions & 2 deletions otherlibs/configurator/test/blackbox-tests/pkg-config-args.t/run.t
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
These tests show that setting `PKG_CONFIG_ARGN` passes extra args to `pkg-config`

$ dune build 2>&1 | awk '/run:.*bin\/pkgconf/{a=1}/stderr/{a=0}a' | sed s/$(ocamlc -config | sed -n "/^target:/ {s/target: //; p; }")/\$TARGET/g
run: $TESTCASE_ROOT/_build/default/.bin/pkgconf --print-errors dummy-pkg
run: $TESTCASE_ROOT/_build/default/.bin/pkgconf --personality $TARGET --print-errors dummy-pkg
-> process exited with code 0
-> stdout:
| --personality
| $TARGET
Copy link
Collaborator

@Leonidas-from-XIV Leonidas-from-XIV Apr 9, 2025

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@MisterDA MisterDA Apr 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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!

Copy link
Collaborator

@Leonidas-from-XIV Leonidas-from-XIV Apr 9, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@MisterDA MisterDA Apr 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@MisterDA MisterDA Apr 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

| dummy-pkg
run: $TESTCASE_ROOT/_build/default/.bin/pkgconf --personality $TARGET --cflags dummy-pkg
-> process exited with code 0
Expand All @@ -22,9 +24,10 @@ These tests show that setting `PKG_CONFIG_ARGN` passes extra args to `pkg-config

$ dune clean
$ PKG_CONFIG_ARGN="--static" dune build 2>&1 | awk '/run:.*bin\/pkgconf/{a=1}/stderr/{a=0}a'
run: $TESTCASE_ROOT/_build/default/.bin/pkgconf --print-errors dummy-pkg
run: $TESTCASE_ROOT/_build/default/.bin/pkgconf --static --print-errors dummy-pkg
-> process exited with code 0
-> stdout:
| --static
| dummy-pkg
run: $TESTCASE_ROOT/_build/default/.bin/pkgconf --static --cflags dummy-pkg
-> process exited with code 0
Expand Down
Loading