CI: Enable eCAP and Valgrind in layer build tests if possible

[eduard-bagdasaryan]

Now these optional features are enabled during applicable layer tests if
their packages appear to be available on the build system. This should
help prevent regressions like the one fixed in recent commit 53ed1a9.

[rousskov]

Thank you for improving our build tests.

[rousskov]

FWIW, I could not find other optional features that satisfy all of these criteria:

• require dependencies that are likely to be missing in many build environments, preventing their unconditional enabling in layers (e.g., installed libecap)
• disabled by default (e.g., eCAP requires --enable-ecap)
• likely to build successfully when a simple pkg-config test passes (e.g., pkg-config --exists valgrind)

If there are such features not covered by this PR, they should be added.

This comment does not request any changes.

[kinkie]

Please emit a prominent warning message before starting a build in case a tests is not performed.
This way we will know well what feature set is being tested, without needing to spelunk hidden log messages

[rousskov]

Please emit a prominent warning message before starting a build in case a tests is not performed.

Based on a singular "warning" wording and "[PR feature presence??] tests not perform" use case, I assume that you are requesting a warning about missing or otherwise unusable pkg-config executable. If that assumption is wrong, please completely ignore the text below and detail your change request. Otherwise, please keep reading.

---

We have already added such a "prominent warning" in commit ea10f7d. Why add a second warning about the same "pkg-config is missing" problem?

This way we will know well what feature set is being tested

AFAICT, we will not: A "pkg-config is missing" warning does not tell us what feature set is being tested. It only tells us that pkg-config is not available. The side effects of that fact are complex (and changing beyond this script knowledge/control).

Overall, determining "feature set" is just too complex of a task to be completed based on any general warning. One pretty much has to "spelunk log messages" to get a correct answer. I think that is OK, and any related "you are (not) building with feature X" reporting improvements should go inside configure.ac rather than external basic scripts like buildtest.sh.

[yadij]

You both appear to be talking about different things. I interpret @kinkie's request as being to add a warning that feature X is skipped. Not about the pkg-config (mandatory build dependency already).

IMO valgrind is special case, being a very expensive thing to test with. As such it should remain a manual operation to build+test with it enabled. So I do support it being added by this PR, but think it should be controlled by a CLI option to the test-builds.sh script.

[kinkie]

You both appear to be talking about different things. I interpret @kinkie's request as being to add a warning that feature X is skipped. Not about the pkg-config (mandatory build dependency already).

IMO valgrind is special case, being a very expensive thing to test with. As such it should remain a manual operation to build+test with it enabled. So I do support it being added by this PR, but think it should be controlled by a CLI option to the test-builds.sh script.

Yes, this

[rousskov]

IMO valgrind is special case, being a very expensive thing to test with

Adding valgrind to maximus layers is not prohibitively expensive for test-builds.sh. In fact, it should have negligible impact because test-builds.sh does not run Squid under valgrind control, and any differences in build speeds from internal cbdata.cc and similar source code "changes" (that --with-valgrind-debug build triggers) ought to be negligible.

When Squid is executed under Valgrind control, it becomes a lot slower indeed, but that expense is not relevant in this PR context.

[kinkie]

Suggested change

	then
	CONFIGURE_FLAGS_MAYBE_ENABLE_ECAP="--enable-ecap"
	fi
	then
	CONFIGURE_FLAGS_MAYBE_ENABLE_ECAP="--enable-ecap"
	else
	echo "NOTICE: Ecap testing disabled" >2
	fi

[eduard-bagdasaryan]

Note however, that the Ecap testing disabled may be a little misleading because it will appear in both situations, i.e., when either pkg-config does not exist or the specified library does not exist. Also it may be irrelevant if the corresponding layer is not configured to build with eCAP.

[rousskov]

Francesco: echo "NOTICE: Ecap testing disabled" >2

Eduard: Ecap testing disabled may be a little misleading because it will appear in both situations, i.e., when either pkg-config does not exist or the specified library does not exist.

... or when the library exists but has a different version. I do not think that fact makes the suggested warning particularly misleading though -- eCAP testing is indeed disabled in all those cases.

N.B. The suggested message itself needs work, but I am ignoring those secondary concerns for now. I will commit a different version if Francesco insists on adding warnings.

Also it may be irrelevant if the corresponding layer is not configured to build with eCAP.

That is a valid concern indeed. The suggested change will create (arguably misleading) noise for layers that are not supposed to test the corresponding feature. Ideally, the warning-printing code should go into each feature-using layer, but that requires more work and will complicate code.

@kinkie, what is your preference among the following options?

1. Merge this PR without additional warnings due to their discussed complications.
2. Add simple warnings, based on your suggested changes, despite those complications.
3. Add layer-driven warnings, despite increased code complexity.

[yadij]

FTR: the build test "layers" are code-coverage tests for compiler identifiable bugs.

Layers other than special-case "layer-00-default" are supposed to be strictly deterministic and code output to be reproducible across build environments. Meaning anything which changes the API behaviour (ie absence of ecap functionality in some environments) is a test FAIL.

The code/binary testing uses the autotools TAP system to verify optional features. Tests are supposed to be reported with a "SKIP" message instead of "FAIL" or "PASS".

[rousskov]

the build test "layers" are code-coverage tests for compiler identifiable bugs.

Each build test layer represents a useful collection of ./configure options (and related settings). The "use case" of each layer is naturally different and layer-specific. The use case for layer-02-maximus is, roughly speaking, "build while enabling everything we can enable". There is even a bit of documentation about that in the layer file itself:

# All configuration options that can be enabled are enabled,
# XXX: with the exception of those that depend on the environment.
# TODO: Add environment-specific tests to enable more options.

This PR reduces the above XXX footprint and partially addresses the above TODO.

The use case for layer-04-noauth-everything -- the other layer affected by this PR -- is defined as "Disable auth and auth helpers. Every other possible feature enabled". That layer comes with the same XXX and TODO that this PR nibbles at.

Layers other than special-case "layer-00-default" are supposed to be strictly deterministic and code output to be reproducible across build environments.

What makes you think that "code output" is "supposed to be reproducible across build environments"? I bet that even before this PR, none of the layers produce the same code across all build environments because there are some environment-specific defaults that Squid uses when built with those layers. In layer files, there are even comments about adjusting layer options based on local conditions. For example (emphasize mine), "Following options should be populated with local settings" and "We can't test them automatically everywhere without detecting those optional packages first". This PR contains that kind of planned "detection" for two packages, moving Squid forward.

Meaning anything which changes the API behaviour (ie absence of ecap functionality in some environments) is a test FAIL.

It is not a FAIL when eCAP is not required to be present in the given environment -- our layers are defined that way. Today, eCAP is optional for all environments that layers test. This PR does not change that and should not change that.

The code/binary testing uses the autotools TAP system to verify optional features. Tests are supposed to be reported with a "SKIP" message instead of "FAIL" or "PASS".

In this context, that assertion applies to building a layer as a whole. This PR does not change how Squid reports existing layer test outcomes. It does not add any SKIP layers either.

[rousskov]

@kinkie, what is your preference among the following options?

1. Merge this PR without additional warnings due to their discussed complications.
2. Add simple warnings, based on your suggested changes, despite those complications.
3. Add layer-driven warnings, despite increased code complexity.

@kinkie, I did not see your response to my question and decided to go with option 2 (i.e. commit your suggestions with minor fixes/adjustments) in hope to move this PR forward. See commit b4eff95. If my earlier arguments convinced you that we should not add these warnings, please let me know, and I would be very happy to remove them.

[yadij]

Please do not force-enable any feature. This particular will break both default and minimal build testing when libecap is installed in the OS for purposes of default build.

How this optional feature is supposed to be done is that each such optional feature has its own "layer-NN-FOO.opts" file(s) defining its build for each permutation of interaction with other options (eg --enable-ecap and --enable-adaptation, --disable-adaptation). Only elide such files if the permutation of build options is already existing in another layer (ie. minimal does the "--disable-adaptation --disable-ecap" case).

IIRC buildtest.sh is currently missing support for a layer .opts file to forcibly skip a test layer that cannot be run. That should be added instead of this.

[rousskov]

Please do not force-enable any feature. This particular will break both default and minimal build testing when libecap is installed in the OS for purposes of default build.

Regarding "force": This PR explicitly enables (under certain conditions and for certain layers) build tests of two default-disabled features. Explicitly enabling default-disabled feature X is the only way to test feature X (by definition of "default-disabled").

This particular will break both default and minimal build testing when libecap is installed in the OS

This PR does not change the set of features enabled during "default and minimal build testing". The changes only affect the set of features that may be enabled in layer-02-maximus and layer-04-noauth-everything (under certain conditions).

You do not have to take my word for it or even study PR diff -- GitHub CI shows successful tests (with eCAP default-disabled, as before) for default and minimal build tests:

BUILD: .././test-suite/buildtests/layer-00-default.opts
... no mention of eCAP-related ./configure checks ...
... no mention of eCAP-related .cc files ...

as well as successful tests (with eCAP enabled) for layer-02-maximus:

BUILD: .././test-suite/buildtests/layer-02-maximus.opts
./../configure  	... --enable-ecap
...
checking for EXT_LIBECAP... yes
...
libtool  ... src/adaptation/ecap/Config.cc

How this optional feature is supposed to be done is that each such optional feature has its own "layer-NN-FOO.opts" file(s) defining its build for each permutation of interaction with other options (eg --enable-ecap and --enable-adaptation, --disable-adaptation). Only elide such files if the permutation of build options is already existing in another layer (ie. minimal does the "--disable-adaptation --disable-ecap" case).

I am not sure we can afford adding permutations with feature-specific build tests, but even if we will be able to agree regarding further build-test enhancements for testing certain features, this simple PR is a significant improvement in test coverage for two maximus layers (that does not preclude those further enhancements in any way). This PR should not be blocked even if we were to agree to add feature-specific layers and permutations in the future.

IIRC buildtest.sh is currently missing support for a layer .opts file to forcibly skip a test layer that cannot be run. That should be added instead of this.

We do not have test layers that cannot be run. If we agree to add such layers in the future, then such a "skip" feature would have to be added indeed, but this PR has nothing to do with that hypothetical future work. This PR improves build test coverage without breaking things or precluding future improvements.

[yadij]

You both appear to be talking about different things. I interpret @kinkie's request as being to add a warning that feature X is skipped. Not about the pkg-config (mandatory build dependency already).

IMO valgrind is special case, being a very expensive thing to test with. As such it should remain a manual operation to build+test with it enabled. So I do support it being added by this PR, but think it should be controlled by a CLI option to the test-builds.sh script.

[rousskov]

Please do not force-enable any feature. This particular will break both default and minimal build testing when libecap is installed in the OS for purposes of default build.

Regarding "force": This PR explicitly enables (under certain conditions and for certain layers) build tests of two default-disabled features. Explicitly enabling default-disabled feature X is the only way to test feature X (by definition of "default-disabled").

This particular will break both default and minimal build testing when libecap is installed in the OS

This PR does not change the set of features enabled during "default and minimal build testing". The changes only affect the set of features that may be enabled in layer-02-maximus and layer-04-noauth-everything (under certain conditions).

You do not have to take my word for it or even study PR diff -- GitHub CI shows successful tests (with eCAP default-disabled, as before) for default and minimal build tests:

BUILD: .././test-suite/buildtests/layer-00-default.opts
... no mention of eCAP-related ./configure checks ...
... no mention of eCAP-related .cc files ...

as well as successful tests (with eCAP enabled) for layer-02-maximus:

BUILD: .././test-suite/buildtests/layer-02-maximus.opts
./../configure  	... --enable-ecap
...
checking for EXT_LIBECAP... yes
...
libtool  ... src/adaptation/ecap/Config.cc

How this optional feature is supposed to be done is that each such optional feature has its own "layer-NN-FOO.opts" file(s) defining its build for each permutation of interaction with other options (eg --enable-ecap and --enable-adaptation, --disable-adaptation). Only elide such files if the permutation of build options is already existing in another layer (ie. minimal does the "--disable-adaptation --disable-ecap" case).

I am not sure we can afford adding permutations with feature-specific build tests, but even if we will be able to agree regarding further build-test enhancements for testing certain features, this simple PR is a significant improvement in test coverage for two maximus layers (that does not preclude those further enhancements in any way). This PR should not be blocked even if we were to agree to add feature-specific layers and permutations in the future.

IIRC buildtest.sh is currently missing support for a layer .opts file to forcibly skip a test layer that cannot be run. That should be added instead of this.

We do not have test layers that cannot be run. If we agree to add such layers in the future, then such a "skip" feature would have to be added indeed, but this PR has nothing to do with that hypothetical future work. This PR improves build test coverage without breaking things or precluding future improvements.

[rousskov]

the build test "layers" are code-coverage tests for compiler identifiable bugs.

Each build test layer represents a useful collection of ./configure options (and related settings). The "use case" of each layer is naturally different and layer-specific. The use case for layer-02-maximus is, roughly speaking, "build while enabling everything we can enable". There is even a bit of documentation about that in the layer file itself:

# All configuration options that can be enabled are enabled,
# XXX: with the exception of those that depend on the environment.
# TODO: Add environment-specific tests to enable more options.

This PR reduces the above XXX footprint and partially addresses the above TODO.

The use case for layer-04-noauth-everything -- the other layer affected by this PR -- is defined as "Disable auth and auth helpers. Every other possible feature enabled". That layer comes with the same XXX and TODO that this PR nibbles at.

Layers other than special-case "layer-00-default" are supposed to be strictly deterministic and code output to be reproducible across build environments.

What makes you think that "code output" is "supposed to be reproducible across build environments"? I bet that even before this PR, none of the layers produce the same code across all build environments because there are some environment-specific defaults that Squid uses when built with those layers. In layer files, there are even comments about adjusting layer options based on local conditions. For example (emphasize mine), "Following options should be populated with local settings" and "We can't test them automatically everywhere without detecting those optional packages first". This PR contains that kind of planned "detection" for two packages, moving Squid forward.

Meaning anything which changes the API behaviour (ie absence of ecap functionality in some environments) is a test FAIL.

It is not a FAIL when eCAP is not required to be present in the given environment -- our layers are defined that way. Today, eCAP is optional for all environments that layers test. This PR does not change that and should not change that.

The code/binary testing uses the autotools TAP system to verify optional features. Tests are supposed to be reported with a "SKIP" message instead of "FAIL" or "PASS".

In this context, that assertion applies to building a layer as a whole. This PR does not change how Squid reports existing layer test outcomes. It does not add any SKIP layers either.

[rousskov]

@kinkie, what is your preference among the following options?

1. Merge this PR without additional warnings due to their discussed complications.
2. Add simple warnings, based on your suggested changes, despite those complications.
3. Add layer-driven warnings, despite increased code complexity.

@kinkie, I did not see your response to my question and decided to go with option 2 (i.e. commit your suggestions with minor fixes/adjustments) in hope to move this PR forward. See commit b4eff95. If my earlier arguments convinced you that we should not add these warnings, please let me know, and I would be very happy to remove them.

[rousskov]

IMO valgrind is special case, being a very expensive thing to test with

Adding valgrind to maximus layers is not prohibitively expensive for test-builds.sh. In fact, it should have negligible impact because test-builds.sh does not run Squid under valgrind control, and any differences in build speeds from internal cbdata.cc and similar source code "changes" (that --with-valgrind-debug build triggers) ought to be negligible.

When Squid is executed under Valgrind control, it becomes a lot slower indeed, but that expense is not relevant in this PR context.

[kinkie]

Thanks for accepting my proposal. LGTM