configure: Add dependency query through pkg-config for libcap

[mobin-2008]

I think it's a good addition to configure.

[davmac314]

From CONTRIBUTING PR-PROCESS:

   See CONTRIBUTING for details. In general new features (and their design) should be agreed upon
   *before* a PR is opened.

Please respect this in the future. I will review this when I have time.

[davmac314]

Please review yourself first and ensure style is consistent with the rest of the code (same file). It seems you haven't done this (eg look at the comments).

This is a requirement for PRs, before you open the PR (or at least before you mark the PR ready for review).

[mobin-2008]

Please review yourself first and ensure style is consistent with the rest of the code (same file). It seems you haven't done this (eg look at the comments).

This is a requirement for PRs, before you open the PR (or at least before you mark the PR ready for review).

Comments? I didn't spot any really significant difference between my code and existing code. I will review it again myself.

[davmac314]

# Test if the compiler accepts an argument (for compilation only); if so add it to named variable.
# Note: this function will return failure if the flag is unsupported (use try_optional_cxx_argument
# instead, to avoid errorring out).
#   $1 - the name of the variable to potentially add the argument to
#   $2 - the argument to test/add
#   CXX - the name of the compiler
#   CXXFLAGS, CXXFLAGS_EXTRA, LDFLAGS, LDFLAGS_EXTRA - any predetermined flags

vs

# Test if the specifced library could be used.
# NOTE: This function will return failure if library is missing, use try_optional_pkgconfig_dependency()
# when library is not required.
# $1 - variable name for passing library linking flags
# $2 - variable name for passing library compiler flags (include directiories etc.)
# $3 - library name
# $4 (Optional) - minimum required version based on pkg-config syntax for version checking.

I would like an answer to this. Are you really not able to see the differences in style between those?

[mobin-2008]

# Test if the compiler accepts an argument (for compilation only); if so add it to named variable.
# Note: this function will return failure if the flag is unsupported (use try_optional_cxx_argument
# instead, to avoid errorring out).
#   $1 - the name of the variable to potentially add the argument to
#   $2 - the argument to test/add
#   CXX - the name of the compiler
#   CXXFLAGS, CXXFLAGS_EXTRA, LDFLAGS, LDFLAGS_EXTRA - any predetermined flags

vs

# Test if the specifced library could be used.
# NOTE: This function will return failure if library is missing, use try_optional_pkgconfig_dependency()
# when library is not required.
# $1 - variable name for passing library linking flags
# $2 - variable name for passing library compiler flags (include directiories etc.)
# $3 - library name
# $4 (Optional) - minimum required version based on pkg-config syntax for version checking.

I would like an answer to this. Are you really not able to see the differences in style between those?

My bad, They are clearly different. I will mark this PR as ready when I double-checked my changes.

[eli-schwartz]

@mobin-2008 thanks for requesting my review, I'm happy / have time to talk about my favorite subject. :) There is a small nuance here that I think you've overlooked.

[eli-schwartz]

This is not going to do what you think it's going to do.

The usual way build systems handle this is by either directly taking dependency names like:

PKG_CHECK_MODULES([LIBCURL], [libcurl >= 8.0])

and passing that as-is to pkg-config --exists, or by taking them as separate arguments and using:

dependency('libcurl', version: '>=9.0')

and translating that to:

# does it exist?
exists = pkg_config.exists('libcurl')

# is it what we want?
sufficient_version, error = pkg_config.modversion('libcurl >= 9.0')
if not sufficient_version:
    print(error)
    # Requested 'libcurl >= 9' but version of libcurl is 8.14.0
    # You may find new versions of libcurl at https://curl.se/

[eli-schwartz]

What you did instead is check modversion without asking pkg-config using the version, and then reimplements version constraint matching using a direct string equals. This will only ever handle asking for exactly a specific micro version -- which is a valid pkg-config use case, but one which people very rarely actually need. It won't handle >= at all, because it will never match the modversion as a string.

[eli-schwartz]

Btw it would probably be remiss of me not to at least point out that as a learning opportunity for writing POSIX shell I guess I don't mind explaining why other build systems act the way they do, but... It's very easy to get this correct with GNU autoconf (which can even produce a configure script that solely outputs an mconfig -- I've done similar things for other projects that need lightweight Makefile config vars but don't use GNU automake).

Because with autotools you just use pkg.m4 and someone else has already done all the work. :D

[mobin-2008]

@mobin-2008 thanks for requesting my review, I'm happy / have time to talk about my favorite subject. :) There is a small nuance here that I think you've overlooked.

Thanks.

What you did instead is check modversion without asking pkg-config using the version, and then reimplements version constraint matching using a direct string equals. This will only ever handle asking for exactly a specific micro version -- which is a valid pkg-config use case, but one which people very rarely actually need. It won't handle >= at all, because it will never match the modversion as a string.

I changed the condition so the pkg-config version checking works correctly now:

    if [ -n "${4:-}" ] && ! "$PKG_CONFIG" --modversion "$3" "$4" >/dev/null 2>&1; then
        sub_info "Incompatible version found: $PKGCONFIG_LIBRARY_VERSION vs $4"
        return 1
    fi

Btw it would probably be remiss of me not to at least point out that as a learning opportunity for writing POSIX shell I guess I don't mind explaining why other build systems act the way they do, but... It's very easy to get this correct with GNU autoconf (which can even produce a configure script that solely outputs an mconfig -- I've done similar things for other projects that need lightweight Makefile config vars but don't use GNU automake).

Because with autotools you just use pkg.m4 and someone else has already done all the work. :D

My problem with autoconf is readability. I saw many projects which have 1000 lines of configure.ac with a mix of ac macros and custom if(s), I believe this script is more readable than many configure.ac files (or maybe I didn't see a well-written configure.ac file).

[eli-schwartz]

I believe this script is more readable than many configure.ac files (or maybe I didn't see a well-written configure.ac file).

https://git.sr.ht/~eschwartz/bzip2/tree/master/item/configure.ac

https://inbox.sourceware.org/bzip2-devel/[email protected]/T/#md9ef9b6a4b342a45d73a385478000a446a6ea755

I've been waiting quite some time for Mark to have time to review this patch, but I'm quite convinced it's a good idea to do. :)

[mobin-2008]

I did some experiments with it, I still don't like its syntax but I will complete it to compare it with this script someday. Thanks for suggestion.

[mobin-2008]

@davmac314 I want to rebase from master branch to my branch for fixing merge conflict with 5a554d8. Can I proceed? (Do draft PRs require permission to rebase?)

[davmac314]

@davmac314 I want to rebase from master branch to my branch for fixing merge conflict with 5a554d8. Can I proceed?

Yes, you can re-base in this instance.

(Do draft PRs require permission to rebase?)

Yes, as documented, in-progress PRs should not be rebased without checking with reviewer first.

Also, it's not really appreciated that you asked a third party to review the PR. No offence to @eli-schwartz and I have no issue with his comments in this particular case, but adding a whole extra review is just more noise that I have to deal with when reviewing, and there is always the possibility that I'm not going to agree with another reviewer.

In future, you should get the PR properly ready before opening it, and follow the documented process. If not, I am going to stop accepting further PRs. If you want feedback from a third party then request that before your open a PR. You can open a PR in your own repository (i.e. a PR which won't appear here in this repo) for that purpose if you want. Do that before you open any PR here.

[davmac314]

The comment example I pointed out still has inconsistency in style. The first comment says "Note: this function will return failure ..." whereas the one you have added says "NOTE: This function will return failure ...". You need to make sure the style is consistent before you ask for review. If this is difficult for you for some reason then you need to:

• take more time with your self-review
• and/or, find or create tooling to help you spot things like this.

Also, the grammar in the comment is not right - it would be worthwhile for you to check your grammar and spelling using a tool (even an online tool). This would be very quick for you to do and would save me from having to rewrite the comment in review.

The commit messages:

 configure: add dependency query through pkg-config

See try_pkgconfig_dependency() and try_optional_pkgconfig_dependency()
functions documention.

and

configure: add libcap dependency query through pkg-config

Check new --enable-capabilities=pkg-config option.

... are both saying to look at the change itself. There is no point having a commit message that just says "look at the change". The commit message is supposed to describe the change. Look at the existing commit messages for examples. This is part of the documented process for contribution.

From a technical perspective, there is an issue with string escaping. It doesn't look like you've really considered this at all. What does pkg-config do about shell metacharacters? Is your code going to handle it properly? Have you tested it? (Eg what if libcap is in a directory containing a space or a wildcard character?)

Why does the try_pkgconfig_dependency support a "required version" parameter if that isn't needed? Have you even tested the functionality? (you obviously hadn't tested it when you first opened the PR because it had a glaring error).

From a feature design perspective, why does the user have to specifically request that pkg-config be used to locate the library? If this is supported, shouldn't it be done by default (if the related variables aren't otherwise specified by the user)?

[eli-schwartz]

From a technical perspective, there is an issue with string escaping. It doesn't look like you've really considered this at all. What does pkg-config do about shell metacharacters? Is your code going to handle it properly? Have you tested it? (Eg what if libcap is in a directory containing a space or a wildcard character?)

Why does the try_pkgconfig_dependency support a "required version" parameter if that isn't needed? Have you even tested the functionality? (you obviously hadn't tested it when you first opened the PR because it had a glaring error).

I asked this question at #465 (comment) and although the code was changed in response to my question, it wasn't actually changed to what I suggested.

So I'm a bit confused here too.

From a feature design perspective, why does the user have to specifically request that pkg-config be used to locate the library? If this is supported, shouldn't it be done by default (if the related variables aren't otherwise specified by the user)?

Agreed. This is something I say all the time when reviewing meson files. :D

The purpose of options is to ask the user to make a decision. Users should not be asked to make meaningless decisions that amount to "tick the box to say yes, you want it to work".

[davmac314]

From a technical perspective, there is an issue with string escaping. It doesn't look like you've really considered this at all. What does pkg-config do about shell metacharacters? Is your code going to handle it properly? Have you tested it? (Eg what if libcap is in a directory containing a space or a wildcard character?)

In fact I really think #471 needs to be addressed, separately, before this PR can continue.

[mobin-2008]

The comment example I pointed out still has inconsistency in style. The first comment says "Note: this function will return failure ..." whereas the one you have added says "NOTE: This function will return failure ...". You need to make sure the style is consistent before you ask for review. If this is difficult for you for some reason then you need to:

* take more time with your self-review

* and/or, find or create tooling to help you spot things like this.

I'm going to use a side-by-side view for reviewing my changes to avoid such mistakes. It looks like getting back and forth in editor is not good enough for me.

Also, the grammar in the comment is not right - it would be worthwhile for you to check your grammar and spelling using a tool (even an online tool). This would be very quick for you to do and would save me from having to rewrite the comment in review.

I will check my comments with tools.

... are both saying to look at the change itself. There is no point having a commit message that just says "look at the change". The commit message is supposed to describe the change. Look at the existing commit messages for examples. This is part of the documented process for contribution.

It will be fixed.

From a technical perspective, there is an issue with string escaping. It doesn't look like you've really considered this at all. What does pkg-config do about shell metacharacters? Is your code going to handle it properly? Have you tested it? (Eg what if libcap is in a directory containing a space or a wildcard character?)

I didn't consider this. As you said, we need to address #471 both in the existing code and this PR.

Why does the try_pkgconfig_dependency support a "required version" parameter if that isn't needed? Have you even tested the functionality? (you obviously hadn't tested it when you first opened the PR because it had a glaring error).

We may require a specific version for a library. For libcap we don't require any specific version for now, but in #400 there is a version check for libselinux. I think this feature of pkg-config should be exposed in our helper function.

I tested it after Eli's review (and I confirm it works). I started working on this feature in https://github.com/mobin-2008/dinit/commits/configure_pkgconfig_dep/ but I did rewrite it in configure_pkgconfig branch and scrapped the selinux check and I forgot to test it again (and it was a mistake).

From a feature design perspective, why does the user have to specifically request that pkg-config be used to locate the library? If this is supported, shouldn't it be done by default (if the related variables aren't otherwise specified by the user)?

Agreed. This is something I say all the time when reviewing meson files. :D

The purpose of options is to ask the user to make a decision. Users should not be asked to make meaningless decisions that amount to "tick the box to say yes, you want it to work".

Yeah it makes sense.

I asked this question at #465 (comment) and although the code was changed in response to my question, it wasn't actually changed to what I suggested.

So I'm a bit confused here too.

# does it exist?
exists = pkg_config.exists('libcurl')

# is it what we want?
sufficient_version, error = pkg_config.modversion('libcurl >= 9.0')
if not sufficient_version:
    print(error)
    # Requested 'libcurl >= 9' but version of libcurl is 8.14.0
    # You may find new versions of libcurl at https://curl.se/

I'm confused. You said I should pass both "library name" and "required version" to pkg-config --modversion and check for the results. Am I got something wrong?

[eli-schwartz]

I'm confused. You said I should pass both "library name" and "required version" to pkg-config --modversion and check for the results. Am I got something wrong?

I must be missing something because I just reread my post and I still can't see where you got the idea that I said any such thing.

[davmac314]

I must be missing something because I just reread my post and I still can't see where you got the idea that I said any such thing

It's probably a mix-up of pkg_config.modversion('libcurl >= 9.0') with pkg-config --modversion. I guess the former is meson syntax? Anyway I can see why that caused confusion.