Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
87b3119
Enable eCAP and Valgrind during CI build-tests
eduard-bagdasaryan Feb 16, 2025
8cb966d
Polished
eduard-bagdasaryan Feb 17, 2025
c87fafb
libecap: specify the minimal required version instead of exact version
eduard-bagdasaryan Feb 17, 2025
a0f0131
Allow the same libecap versions with pkg-config and PKG_CHECK_MODULES
eduard-bagdasaryan Feb 17, 2025
11b5e18
fixup: Install Valgrind to enable the corresponding build tests
rousskov Feb 23, 2025
b4eff95
Warn when build environment does not support an optional feature
kinkie Feb 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions test-suite/buildtest.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,18 @@
action="${1}"
config="${2}"

# Allow a layer to enable optional default-disabled features when
# those features are supported in the current build environment
# (and we can easily detect such support).
if ${PKG_CONFIG:-pkg-config} --exists 'libecap >= 1.0 libecap < 1.1' 2>/dev/null
then
CONFIGURE_FLAGS_MAYBE_ENABLE_ECAP="--enable-ecap"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

fi
Copy link
Contributor

@kinkie kinkie Feb 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@rousskov rousskov Feb 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if ${PKG_CONFIG:-pkg-config} --exists valgrind 2>/dev/null
then
CONFIGURE_FLAGS_MAYBE_ENABLE_VALGRIND="--with-valgrind-debug"
Copy link
Contributor

@kinkie kinkie Feb 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

fi

# cache_file may be set by environment variable
configcache=""
if [ -n "$cache_file" ]; then
Expand Down
4 changes: 2 additions & 2 deletions test-suite/buildtests/layer-02-maximus.opts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,9 @@ MAKETEST="distcheck"
# We can't test them automatically everywhere without detecting those
# optional packages first.
#
# --enable-ecap \
# --enable-epoll \
# --enable-kqueue \
# --enable-win32-service \
# --with-valgrind-debug \
# --with-gnutls \
# --with-tdb \
# --with-cap \
Expand Down Expand Up @@ -110,6 +108,8 @@ DISTCHECK_CONFIGURE_FLAGS=" \
--enable-build-info=squid\ test\ build \
--enable-ssl-crtd \
--with-openssl \
$CONFIGURE_FLAGS_MAYBE_ENABLE_ECAP \
$CONFIGURE_FLAGS_MAYBE_ENABLE_VALGRIND \
"

# Fix the distclean testing.
Expand Down
4 changes: 2 additions & 2 deletions test-suite/buildtests/layer-04-noauth-everything.opts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,9 @@ MAKETEST="distcheck"
# We can't test them automatically everywhere without detecting those
# optional packages first.
#
# --enable-ecap \
# --enable-epoll \
# --enable-kqueue \
# --enable-win32-service \
# --with-valgrind-debug \
# --with-ldap \
Comment on lines 42 to 48
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#
# --enable-cpu-profiling \ Requires CPU support.
Expand Down Expand Up @@ -110,6 +108,8 @@ DISTCHECK_CONFIGURE_FLAGS=" \
--with-pic \
--with-pthreads \
--enable-build-info=squid\ test\ build \
$CONFIGURE_FLAGS_MAYBE_ENABLE_ECAP \
$CONFIGURE_FLAGS_MAYBE_ENABLE_VALGRIND \
"

# Fix the distclean testing.
Expand Down
Loading