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.

[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.

[mobin-2008]

@davmac314 I want to rebase from master branch to my branch for fixing merge conflict with 5a554d8bfce1f5ee280a0f8e8aefe85b8fd1ff69. 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.

[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 https://github.com/davmac314/dinit/pull/465#discussion_r2116564825 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.