Skip to content

Take host into account when looking for pkg-config#12190

Merged
gasche merged 2 commits intoocaml:trunkfrom
shindere:fix-zstd-support-detection
Apr 18, 2023
Merged

Take host into account when looking for pkg-config#12190
gasche merged 2 commits intoocaml:trunkfrom
shindere:fix-zstd-support-detection

Conversation

@shindere
Copy link
Copy Markdown
Contributor

When cross-compiling (e.g. for the Windows ports), one needs to look
for ${host}-pkg-configin priority, if one wants to find the
right instance of pkg-config.

This PR fixes the support for the pkg-config-based detection of zstd.

@gasche
Copy link
Copy Markdown
Member

gasche commented Apr 18, 2023

Two questions:

  1. why do we have [pkg-configx] with a x at the end, is this a typo?
  2. what is the use of the [false] default value at the end, is it meant to be checked explicitly before calling pkg-config in the rest of the configure script? (Or does everything work as expected on systems where pkg-config is not available, because tests using PKG_CONFIG just fail?)

This is to prepare the next commit: when cross-compiling the right
pkg-config to use is the one giving information on the host system.
Try ${host}-pkg-config before falling back to pkg-config
@shindere
Copy link
Copy Markdown
Contributor Author

shindere commented Apr 18, 2023 via email

@shindere shindere force-pushed the fix-zstd-support-detection branch from 28bc7fb to bc9c87d Compare April 18, 2023 15:04
@gasche
Copy link
Copy Markdown
Member

gasche commented Apr 18, 2023

The change looks fine to me after your explanation. Two points:

  1. I think that the CI earlier didn't catch the typo, so I am nervous about disabling the zstd support (at least using pkg-config) without noticing. Could you test manually that the patch behaves as expected on pkg-config-enabled systems?
  2. I wondered if one should leave PKG_CONFIG to its "ambiant" value if the lookup fails (I think this is what happens if you don't provide an else branch to AC_PATH_TOOL?), which lets user configure the value. I think that the current proposal to use [false] is simpler and better -- introducing another configuration point is best done different, is work, and we should wait to see if there is a need.

@shindere
Copy link
Copy Markdown
Contributor Author

shindere commented Apr 18, 2023 via email

Copy link
Copy Markdown
Member

@gasche gasche left a comment

Choose a reason for hiding this comment

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

Approved, but we should wait for @shym to manually test the patch before merging, as this feature is not exercised in our CI and I have not tested myself.

@shym
Copy link
Copy Markdown
Contributor

shym commented Apr 18, 2023

I've been bitten by this in the context of compiling a 32-bit OCaml on
an amd64 machine (in CI).

In this setting the options we have to consider are whether pkg-config
and zstd are installed in 32-bit and / or 64-bit versions. As the
64-bit versions are installed by default in CI, I tested the
combinations of installations of the 32-bit versions:

So this is a nice improvement, thank you!

@shym
Copy link
Copy Markdown
Contributor

shym commented Apr 18, 2023

I should have added that the interesting part to look into the logs, as far as this PR is concerned, is probably the "Show configuration and clean up" step, that displays ocamlc -config.

@shym
Copy link
Copy Markdown
Contributor

shym commented Apr 18, 2023

If I understand correctly, if we could say ./configure PKG_CONFIG=..., the opam package could enforce the version of PKG_CONFIG to use when the ocaml-option-32bit is installed. I'm not sure what error message (between the one at linking and the one at configure) would be easier to understand for end-users, though.

@gasche
Copy link
Copy Markdown
Member

gasche commented Apr 18, 2023

Thanks @shym for the tests, merging now.

@gasche gasche merged commit 2725f08 into ocaml:trunk Apr 18, 2023
gasche added a commit that referenced this pull request Apr 18, 2023
Take host into account when looking for pkg-config

(cherry picked from commit 2725f08)
@shindere shindere deleted the fix-zstd-support-detection branch April 19, 2023 09:52
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.

3 participants