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

Configurator: pkg-config plugin uses pkgconf and --personality=TARGET by default #10937

Merged
merged 5 commits into from
Nov 1, 2024

Conversation

pirbo
Copy link
Contributor

@pirbo pirbo commented Sep 19, 2024

This change is required to support windows mingw opam.2.2 default setting. It can be seen as a "clean" attempt to do dra27@fd003a0 :-)

Maybe review commit by commit as commit messages tries to provide explanations.

Obviously the last one (refreshing test outputs) is not satisfactory as it but as I don't know what is the best fix, I open the PR now as an ask for feedback...

@pirbo pirbo marked this pull request as ready for review October 15, 2024 08:53
@Leonidas-from-XIV
Copy link
Collaborator

Could you add a changelog entry and potentially some documentation of the behavior so people are aware of it?

@rgrinberg
Copy link
Member

Making the tests pass would be appreciated as well

otherlibs/configurator/src/v1.ml Outdated Show resolved Hide resolved
otherlibs/configurator/src/v1.ml Outdated Show resolved Hide resolved
(name pkg_config)
(modules pkg_config))
(name pkgconf)
(modules pkgconf))
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be nice if both dummy pkg_config and pkgconf were both built to be able to demonstrate the fallback behavior in tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put it some thought even before your remark and that fallback is subtle to demonstrate as it requires to remove the pkgconf you have on your system from your PATH else it is favored over the local pkg-config...
If you figure out a way, I can implement it :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see your point, there is no elegant solution to remove thing from the PATH. That said:

It is a bit of a hassle but you could in the test adjust PATH to only contain a directory you created that contains symlinks to the binaries you expect (e.g. iterate over the PATH that the tests starts with, symlink everything but pkgconf to make sure it still works if pkgconf/pkg-config calls other binaries) and then use that filtered directory as sole PATH for the rest of the test.

@pirbo pirbo force-pushed the pirbo-pkgconf branch 2 times, most recently from 5a7ac12 to 0115654 Compare October 18, 2024 11:26
@pirbo
Copy link
Contributor Author

pirbo commented Oct 18, 2024

Documentation updated (and of course not promoting the next test output was a glitch...)
Edit: Sorry, I forgot to thank you for your reviews... :-) Thank you!

@pirbo
Copy link
Contributor Author

pirbo commented Oct 18, 2024

I've pushed a fixup commit (that I'll of course squash if it works) to try blindly to please MacOS sed...
If not, I'll only have an hand on a mac to try on my side tonight...

@rgrinberg
Copy link
Member

@anmonteiro could we test this one on nix?

@@ -1,34 +1,40 @@
These tests show that setting `PKG_CONFIG_ARGN` passes extra args to `pkg-config`

$ dune build 2>&1 | awk '/run:.*bin\/pkg-config/{a=1}/stderr/{a=0}a'
run: $TESTCASE_ROOT/_build/default/.bin/pkg-config --print-errors dummy-pkg
$ 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
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't this rather be added to the BUILD_PATH_PREFIX_MAP variable at the top of this file?

Copy link
Collaborator

Choose a reason for hiding this comment

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

check some of the other tests for how that's done. It feels a bit cleaner to me to do it that way

Copy link
Contributor Author

@pirbo pirbo Oct 29, 2024

Choose a reason for hiding this comment

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

How ? Please help! :-)
This was my first idea of course. I never managed to make it work. That's why I fold back to a sed command...
After your comment, I said to myself to not be lazy and to lead this battle but I'm defeated again and again :-/
I tried all the variant I could imagine of

-  $ 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
+  $ target=$(ocamlc -config | sed -n "/^target:/ {s/target: //; p; }")
+  $ printf $target
+  x86_64-pc-linux-gnu
+  $ export BUILD_PATH_PREFIX_MAP="$BUILD_PATH_PREFIX_MAP:TARGET=$target"
+  $ dune build 2>&1 | awk '/run:.*bin\/pkgconf/{a=1}/stderr/{a=0}a'

the outcome keeps being

-  run: $TESTCASE_ROOT/_build/default/.bin/pkgconf --personality $TARGET --cflags dummy-pkg
+  run: $TESTCASE_ROOT/_build/default/.bin/pkgconf --personality x86_64-pc-linux-gnu --cflags dummy-pkg
   -> process exited with code 0
   -> stdout:
    | --personality
-   | $TARGET
+   | x86_64-pc-linux-gnu
    | --cflags
    | dummy-pkg
-  run: $TESTCASE_ROOT/_build/default/.bin/pkgconf --personality $TARGET --libs dummy-pkg
+  run: $TESTCASE_ROOT/_build/default/.bin/pkgconf --personality x86_64-pc-linux-gnu --libs dummy-pkg
   -> process exited with code 0
   -> stdout:
    | --personality
-   | $TARGET
+   | x86_64-pc-linux-gnu
    | --libs
    | dummy-pkg

@anmonteiro
Copy link
Collaborator

@anmonteiro could we test this one on nix?

Naively this looks good to me but I still need to properly test it. Please allow a couple days for me to take this for a spin.

anmonteiro added a commit to nix-ocaml/nix-overlays that referenced this pull request Oct 20, 2024
@anmonteiro
Copy link
Collaborator

kicked off a test run here: nix-ocaml/nix-overlays#1717

we build a few packages that rely on pkg-config, also for x-compilation.

This makes the implementation of `which` in configurator closer to the one of stdune.

Signed-off-by: Pierre Boutillier <pierre.boutillier@laposte.net>
Pkgconf is a modern implementation of the "pkg-config freedesktop standard". Linux distros actually use pkgconf as their
pkg-config implementation and provide a in place replacement. (Homebrew packages both and is the last place I found where you can get the historical implementation).
It is always OK to call pkgconf for pkg-config all the CLI and outputs within the standard are following the standard :-).

Cygwin pkg-config package though provides pkgconf but does not provide the alias pkg-config forcing us to uptream the switch to the tool name instead of the standard.

Signed-off-by: Pierre Boutillier <pierre.boutillier@laposte.net>
Signed-off-by: Pierre Boutillier <pierre.boutillier@laposte.net>
Specifying a personality is only useful when cross-compiling (which you do when you use mingw on Windows) but
(nearly) never harmful as pkgconf installs a default.personality which it uses for all triplets (that parses
therefore the nearly) it hasn't a explicit personality for.

Signed-off-by: Pierre Boutillier <pierre.boutillier@laposte.net>
Signed-off-by: Pierre Boutillier <pierre.boutillier@laposte.net>
anmonteiro added a commit to nix-ocaml/nix-overlays that referenced this pull request Oct 30, 2024
@rgrinberg rgrinberg merged commit f951a75 into ocaml:main Nov 1, 2024
27 checks passed
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.

4 participants