Fix PKG_CHECK_MODULES interaction with environment variables

[yadij]

Testing has shown that this macro behaviour does not meet
several of our desired ./configure behaviours despite still
being the most portable way to detect library build details.

Specifically, to meet the desired Squid build behaviour of
obeying environment supplied LIBFOO_* variables we must
supply no-op values for both the action-if-found and
action-if-not-found parameters. Then perform any additional
checks (such as header detection) separately.

Additional checks may depend on the LIBFOO_LIBS variable, but
must not depend on anything inside the PKG_CHECK_MODULES action
parameters being executed.

[kinkie]

👍🏼

[rousskov]

PR description: ... the desired Squid build behaviour of obeying environment supplied LIBFOO_* variables ...

Let's see if there is consensus regarding what that "desired Squid build behaviour" is. I suspect there is no such consensus, and it is important to get this right if we are going to use that desire to justify significant configure.ac changes such as those contained in this PR. Please define what that desired behavior is in your opinion (or point me to the existing statement explicitly defining it).

PR description: to meet the desired Squid build behaviour of obeying environment supplied LIBFOO_* variables we must supply no-op values for both the action-if-found and action-if-not-found parameters.

Why? I see no connection between "obeying supplied LIBFOO_" and "must supply no-op values for both the action-if-found and action-if-not-found parameters". Certainly, many values other than no-op will have the same effect on "obeying supplied LIBFOO_" goal as no-op values. Either this "must" is excessively restrictive or additional goals should be disclosed to justify it.

This change request is not bickering about a minor wording problem. It is important to get this right because specifying an action-if-not-found parameter has profound effects on PKG_CHECK_MODULES() calls -- adjusted calls stop killing ./configure on certain failures: "the default action-if-not-found will end the execution with an error for not having found the dependency".

[yadij]

We already discussed and settled these details two weeks ago at the weekly meeting.

PR description: ... the desired Squid build behaviour of obeying environment supplied LIBFOO_* variables ...

Let's see if there is consensus regarding what that "desired Squid build behaviour" is.

• Our desire is that:
  . the admin can provide custom LIBFOO_{CFLAGS,LIBS,PATH} environment variables and our checks for specific library headers, functions, API symbols etc are performed with those values - same as if pkg-config had supplied the variables.

PR description: to meet the desired Squid build behaviour of obeying environment supplied LIBFOO_* variables we must supply no-op values for both the action-if-found and action-if-not-found parameters.

Why? I see no connection between "obeying supplied LIBFOO_" and "must supply no-op values for both the action-if-found and action-if-not-found parameters".

If you recall @kinkie's tests during the meeting: neither action-if-found nor action-if-not-found logic were actually happening when the admin provides LIBFOO_LIBS via the environment or ./configure command line.

Certainly, many values other than no-op will have the same effect on "obeying supplied LIBFOO_" goal as no-op values. Either this "must" is excessively restrictive or additional goals should be disclosed to justify it.

There are three types of value that can be provided to both the PKG_CHECK_MODULES action parameters:

1. the [] unset/empty value - this will halt the ./configure script with an error when the library is supposed to be optional and auto-detected, but is not found. Breaking a significant subset of expected build cases.
2. the [:] no-op value - this will fall through to continue with the next ./configure.ac action. This is reliable for all build cases.
3. some logic - this will only be executed in the absence of admin providing environment variables. As such it cannot be relied on happening for a significant subset of expected build cases.

Logic existence is not prohibited, relying on it is variously broken.

[rousskov]

We already discussed and settled these details two weeks ago at the weekly meeting.

We discussed related topics, but settlement of complex technical issues usually requires written text, especially when folks involved often interpret the same discussion very differently. This case is no exception.

Our desire is that: the admin can provide custom LIBFOO_{CFLAGS,LIBS,PATH} environment variables and our checks for specific library headers, functions, API symbols etc are performed with those values - same as if pkg-config had supplied the variables.

This is a good start, thank you, but this is not enough to reach consensus. Besides minor technical corrections, we need to decide whether custom variables provided by the admin indicate that the admin wants to enable the corresponding feature. I suspect the best answer is "no, they do not" and can suggest the following text for the next round, but there are probably other reasonable ways to structure this:

1. An admin may provide either both or none of the pkg-config environment variables to ./configure: LIBFOO_LIBS and LIBFOO_CFLAGS. Providing only one of the variables leads to undefined behavior.
2. Providing pkg-config variables does not indicate admin desire to enable (or disable) the feature: The admin must use the corresponding ./configure command line parameters to alter Squid defaults (e.g., --with-foo or --enable-bar).
3. If an admin provides pkg-config environment variables, then pkg-config is not going to check whether package version requirements have been met. Providing environment variables that point to an unsupported (by Squid) package leads to undefined behavior.
4. Whether pkg-config environment variables are provided or not, Squid may check other (version-unrelated) conditions that control, for example, whether the corresponding feature is enabled or disabled. Those additional checks should not treat pkg-config environment variables differently from pkg-config-computed ones.

I hope the above can be simplified, but that is the simplest version I can propose right now.

XXX: The above does not tell us whether Squid may overwrite admin-provided pkg-config environment variables.

AFAICT, the above rules lead to Squid code using the following two PKG_CHECK_MODULES call formats, depending on the context of the call.

dnl A: Testing whether a default-enabled module is present:
dnl Testing that the module was not explicitly disabled is done earlier and is not shown here.
PKG_CHECK_MODULES(FOO, MODULES, [:], [:])

dnl  B: Testing whether a required (e.g., by `--enable-foo`) module is present.
dnl Testing that the module was required is done earlier and is not shown here.
PKG_CHECK_MODULES(FOO, MODULES, [:])

The "B" sketch could use [] for the fourth parameter, but I think the missing parameter is a bit easier to spot than the difference between [] and [:]. This is a minor/secondary style issue.

In addition to the above, to satisfy item 4, Squid should never test/use pkg-config variables for FOO before calling PKG_CHECK_MODULES(FOO).

PR description: to meet the desired Squid build behaviour of obeying environment supplied LIBFOO_* variables we must supply no-op values for both the action-if-found and action-if-not-found parameters.

Alex: Why? I see no connection between "obeying supplied LIBFOO_" and "must supply no-op values for both the action-if-found and action-if-not-found parameters".

Amos: If you recall @kinkie's tests during the meeting: neither action-if-found nor action-if-not-found logic were actually happening when the admin provides LIBFOO_LIBS via the environment or ./configure command line.

Yes, I do recall, and pkg-config documentation/output says the same. I still see no connection though. There would be a connection if that statement said "must supply a no-op value for the action-if-found parameter" instead. The action-if-not-found parameter is a different story (see item 2 and the corresponding sketch "B" above).

We do not want to break --enable-foo support that relies on PKG_CHECK_MODULES() check failing (when no environmental variables were specified). There may be related bugs in the existing code already, but we do not want to create more.

[yadij]

Wow. So much bureaucracy and mandatory justifications. You said something about not quibbling over wording, now this.

Please, go right ahead and paste that B sketch into configure.ac (anywhere after the top m4_include list will do).

Anyway, for the record; what I am working with is a first requirement that the library check for existence of optional auto-detected libraries does not halt ./configure with an error. So:

PKG_CHECK_MODULES(FOO, MODULES) 👎

checking for FOO... no
configure: error: Package requirements (MODULES) were not met:

PKG_CHECK_MODULES(FOO, MODULES, ) 👎

checking for FOO... no
configure: error: Package requirements (MODULES ) were not met:

PKG_CHECK_MODULES(FOO, MODULES,,) 👎

checking for FOO... no
configure: error: Package requirements (MODULES ) were not met:

PKG_CHECK_MODULES(FOO, MODULES, []) 👎

checking for FOO... no
configure: error: Package requirements (MODULES ) were not met:

PKG_CHECK_MODULES(FOO, MODULES, [:]) 👎

checking for FOO... no
configure: error: Package requirements (MODULES) were not met:

PKG_CHECK_MODULES(FOO, MODULES, [], []) 👎

checking for FOO... no
configure: error: Package requirements (MODULES) were not met:

PKG_CHECK_MODULES(FOO, MODULES, [:], []) 👎

checking for FOO... no
configure: error: Package requirements (MODULES) were not met:

PKG_CHECK_MODULES(FOO, MODULES, [], [:]) 👍

checking for FOO... no
checking build system type... x86_64-pc-linux-gnu
...

PKG_CHECK_MODULES(FOO, MODULES, [:], [:]) 👍

checking for FOO... no
checking build system type... x86_64-pc-linux-gnu
...

Now that we know which produce a "no" without halting we can test which of those (2 final options) will be usable for testing extra things. As already mentioned the PKG_CHECK_MODULES(FOO, MODULES, [BLAH], [:]) does not run BLAH logic reliably.

PKG_CHECK_MODULES(FOO, [pam], [AC_CHECK_HEADERS(pam.h)], [:]) :

This is where the test results vary. For me with pkg-config version 1.8.1 I get these:

• ./configure

checking for FOO... yes
checking for pam.h... no

• ./configure FOO_LIBS=bar

checking for FOO... yes
checking for pam.h... no

Whereas @kinkie in the meeting demonstrated other OS we support producing:

• ./configure

checking for FOO... yes
checking for pam.h... no

• ./configure FOO_LIBS=bar

checking for FOO... yes

[rousskov]

Wow. So much bureaucracy and mandatory justifications. You said something about not quibbling over wording, now this.

I am not sure whether your response should be interpreted as "I am OK with your version despite its shortcomings", or "I am not OK with your version, and am not going to cooperate on developing a version that we all can support", or something else. Please clarify.

Please, go right ahead and paste that B sketch into configure.ac (anywhere after the top m4_include list will do).

The equivalent code already exists in master and works correctly for the primary use case broken by the current PR changes. I have tested both master and PR code.

Anyway, for the record; what I am working with is a first requirement that the library check for existence of optional auto-detected libraries does not halt ./configure with an error.

I am aware of that. The primary problem here is not with the (desirable but secondary) functionality you are working on. The problem is that this PR is breaking another (desirable and primary) functionality in the process. I tried to formulate the invariants and provide the corresponding sketches, so that you can use them to fix this PR (at least), but that plan failed. After you answer the question in this comment, I will come up with plan B.