8000 Correctly check for pkg-config by sardemff7 · Pull Request #1885 · php/php-src · GitHub
[go: up one dir, main page]
Skip to content

Correctly check for pkg-config#1885

Closed
sardemff7 wants to merge 1 commit intophp:masterfrom
sardemff7:wip/pkg-config
Closed

Correctly check for pkg-config#1885
sardemff7 wants to merge 1 commit intophp:masterfrom
sardemff7:wip/pkg-configsardemff7/php-src:wip/pkg-config

Conversation

@sardemff7
Copy link
@sardemff7 sardemff7 commented Apr 22, 2016 •
edited
Loading

It is a tool, and needs the correct prefix for cross-compiling.
Also, if $PKG_CONFIG is set, either by the user or AC_PATH_TOOL, we can
consider it will be executable.
A bare test -x is not enough for package managers which set
$PKG_CONFIG to <prefix>-pkg-config (not absolute).

@sardemff7
Correctly check for pkg-config
8ec9a74 
It is a tool, and needs the correct prefix for cross-compiling.
Also, if $PKG_CONFIG is set, either by the user or AC_PATh_TOOL, we can
consider it will be executable.
A bare "test -x" is not enough for package managers which set
$PKG_CONFIG to <prefix>-pkg-config (not absolute).

Signed-off-by: Quentin Glidic <sardemff7+git@sardemff7.net>
@krakjoe
Copy link
Member
krakjoe commented Jan 7, 2017

@remicollet can I ask that you review this please ?

@orlitzky
Copy link
Contributor
orlitzky commented Jul 19, 2017

You switched from -x to -n to catch the case where $PKG_CONFIG is not an absolute path, but isn't the AC_PATH_TOOL macro guaranteed to give you an absolute path?

@orlitzky
Copy link
Contributor
orlitzky commented Jul 19, 2017

Nevermind, I see it now: the AC_PATH_TOOL macro is only used when $PKG_CONFIG is empty. I guess you can define $PKG_CONFIG yourself to force a particular implementation? In that case, I would probably either resolve a relative path to an absolute one, or simply state that $PKG_CONFIG is an absolute path and let the customer figure it out.

@sardemff7
Copy link
Author
sardemff7 commented Jul 19, 2017

$PKG_CONFIG can be used when cross-compiling with an already existing tool that exports it.
There is no value to checking if $PKG_CONFIG can run, since AC_PATH_TOOL guarantees that, and only specific tools and package managers set the variable directly. If the user sets it to some silly value, the call right after the && will catch the error.
Requiring the -x test means modifying existing tools to set the absolute path or not to export the variable, while only these tools (made specifically to cross-compile, and thus knowing what they do to the variable) are likely to use this variable.

What kind of failures are you trying to catch here? This check just seems annoying, since every other good user of Autotools just use the PKG_PROG_PKG_CONFIG macro which allows that behaviour.

@orlitzky
Copy link
Contributor
orlitzky 8000 commented Jul 19, 2017

You're right that the actual invocation of pkgconfig will fail if it's not executable, I just think it's a slightly better design to decide up front what the $PKG_CONFIG variable should contain. If we say it contains an absolute path, then either the user has to give us one, or we have to convert relative to absolute early on. But from that point on, the script can rely on the fact that $PKG_CONFIG is an absolute path.

If, on the other hand, we let $PKG_CONFIG be either relative or absolute, then every place in the script that it's used needs to be aware of that fact and handle both cases. I think this approach is more likely to cause bugs in the future, if (for example) somebody adds another line like the -x test that we have now -- which somebody might do completely independently five years from now, not realizing that the path can be relative.

I just happened across this issue by accident and decided to look over the change, so my suggestion isn't the law =)

I do however agree with the goal.

@sardemff7
Copy link
Author
sardemff7 commented Jul 19, 2017

I see. However, $PKG_CONFIG is a standard variable used across countless projects, just based on the upstream PKG_PROG_PKG_CONFIG macro (used by PKG_CHECK_MODULES and PKG_CHECK_EXISTS internally at least).
Let’s see what other peoples think about it.

@krakjoe krakjoe requested a review from remicollet July 20, 2017 07:25
@KalleZ
Copy link
Member
KalleZ commented Aug 22, 2017

@remicollet with the recent changes to use pkg-config in curl I think it was, I suppose this could be merged as well?

@cmb69
Copy link
Member
cmb69 commented Nov 27, 2018

@eli-schwartz Is this PR obsolete due to PR #3632 and #3654?

@eli-schwartz
Copy link
Contributor
eli-schwartz commented Nov 28, 2018

Yes.

  • calls to AC_PATH_PROG are obsolete since PKG_PROG_PKG_CONFIG is run during early configure

  • $PKG_CONFIG --exists can regardless be replaced universally with PKG_CHECK_EXISTS

  • these checks should be (and in some cases have been) recreated with PKG_CHECK_MODULES, which combines PKG_CHECK_EXISTS and the actual definition of various *_CFLAGS variables

@sardemff7 sardemff7 closed this Nov 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@remicollet remicollet Awaiting requested review from remicollet

Assignees

No one assigned

Labels

Category: Build System Waiting on Review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@sardemff7 @krakjoe @orlitzky @KalleZ @cmb69 @eli-schwartz @laruence

Comments
0