Document ideal configure.ac structure#2341
Document ideal configure.ac structure#2341yadij wants to merge 27 commits intosquid-cache:masterfrom
Conversation
configure.ac
Outdated
| ## | ||
|
|
||
| # | ||
| # The layout of this file is intended to be modular with |
There was a problem hiding this comment.
It is not clear to me what "modular layout" implies in this context. Please drop that phrase or rephrase to clarify.
configure.ac
Outdated
| # 2. Toolchain detection | ||
| # | ||
| # 3. Build Environment detection | ||
| # | ||
| # 4. Library detection | ||
| # | ||
| # 5. Feature detection |
There was a problem hiding this comment.
This is the primary change request.
The difference/scope of these items is unclear to me. For example, I expect "build environment" to include "toolchain" (listed above) and "library" presence (listed below). These items are further detailed down below, but those details do not help enough to address this concern and raise additional difference/dependency/scope red flags.
If we want to sort ./configure items, then let's start with what non-obvious rules or principles/ideas we want that order to enforce (instead of starting with a specific list that reflects some rules and ideas unknown to PR readers). In other words, propose the comparison function rather than the result of sorting with a comparison function unknown to the reader...
Does autoconf documentation offer any relevant advice?
There was a problem hiding this comment.
The difference/scope of these items is unclear to me.
I tried to define these titles here, but found myself partially repeating the texts later anyway.
To fix that risk I chose to make these lines a simple table of contents. Definitions, requirements for items in the section, and documented reasons for section existence are written as a single prefix text in each section.
For example, I expect "build environment" to include "toolchain" (listed above) and "library" presence (listed below). These items are further detailed down below, but those details do not help enough to address this concern and raise additional difference/dependency/scope red flags.
Nod. Will fix those detail documentation blurbs as necessary to clarify. Most I am happy doing here is something to clarify that this list is a ToC/index of sections.
If we want to sort
./configureitems, then let's start with what non-obvious rules or principles/ideas
Of course. This is what I have tried to document via this PR description. If there are any you object to, or think are missing please point out as review requests to add/discuss. The docs added here kind of need to be consensus before this PR merges :-)
Does autoconf documentation offer any relevant advice?
Unfortunately no advice I can see from autoconf documentation beyond the header/footer macros for aclocal setup and finalize. Those already exist properly and are not being touched here. What happens between is entirely up to us.
configure.ac
Outdated
| # Identify any special parameters need to run the tools correctly. | ||
| # For example; "rm" vs "rm -f", "ar" vs "ar r", "grep -E" vs "egrep", or "-std=17" |
There was a problem hiding this comment.
But these "special parameters" may depend on "build environment" that is detected later on. Even a tool itself may be environment-specific.
There was a problem hiding this comment.
My concept for these at present is that this Toolchain section detects the mandatory program names and flags.
The user should not be casually setting things (can via environment, but should not).
The later Environment section has a lot of overlap, but all things in there are essentially "optional" and may be altered by the user easily with --enable/--with controls.
configure.ac
Outdated
| # All tests in later sections should be able to include "compat/" | ||
| # files and use their definitions. |
There was a problem hiding this comment.
We should not use compat/ sources in ./configure checks because that would imply that compat/ sources cannot reliably/safely use ./configure results. I hope such use is unnecessary. I suggest studying a specific example that does, in your opinion, require such use.
There was a problem hiding this comment.
My reason for this is to avoid duplicating the autoconf checks logic in both the AC test and compat/ files. We have repeatedly had portability issues with the two locations getting out of sync.
configure.ac
Outdated
| # Which libraries are available will determine which features | ||
| # can be enabled later. |
There was a problem hiding this comment.
I doubt we should segregate --with-foo from --enable-bar. Both can be treated as "features", and there is often no need for default-enabled library checks if a certain feature is disabled (and vice versa).
There was a problem hiding this comment.
I have segregated with libraries first because many libraries are shared between multiple features. Also, it makes more sense to produce the "cannot enable feature" error message when checking the --enable than when checking the library after already having enabled the feature build.
Both can be treated as "features"
Per autoconf documentation;
- section 15.2
--withis for external software packages (libraries or programs). - section 15.3
--enableis for optional features of the software being built.
There was a problem hiding this comment.
I have been giving this more thought in the past weeks and figured how to cleanly swap the order. It is a bit more complex than I wanted, but doable.
The value of this has not been meaningful since we stopped committing ./configure to VCS over 15 years ago.
Fix missing header `sys/socket.h`:
configure:29217: checking for struct arpreq.arp_pa
.../net/if_arp.h: error: field has incomplete type 'struct sockaddr'
Fix `ac_aggr.arp_pa` test, replacing it with a test of a data member
that Squid code is actually using:
conftest.cpp:112:5: error: value of type 'struct sockaddr' is not
contextually convertible to 'bool'
Discovered while porting Squid v7 to FreeBSD.
To display The Squid Software Foundation copyright blurb in ./configure --version output and mentions at the top of ./configure script code.
From automake manual: " This is an obsolescent macro that checks that the C compiler supports the -c and -o options together. Note that, since Automake 1.14, the AC_PROG_CC is rewritten to implement such checks itself, and thus the explicit use of AM_PROG_CC_C_O should no longer be required. " This also changes an implicit build requirement of automake 1.5 to an explicit 1.14 or later requirement.
When a unit test fails during CI checks, the corresponding GitHub
Actions reports and collected btlayer-*.log files do not contain
any failure details. For example, we see
FAIL: tests/testRock
# FAIL: 1
but are missing critical details like
stub time| FATAL: Ipc::Mem::Segment::create failed to
shm_open(/squid-0-tr_rebuild_versions.shm): (63) File name too long
Now, GitHub Actions collect all log files, including unit test logs. For
the `ubuntu-24.04,gcc,default` build target, adding more logs increases
artifacts zip archive size by about 100 KB (from ~200KB to ~300KB).
If `test-builds.sh` succeeds, there are no unit test logs to collect
because all unit test logs are erased when `make distcheck` (initiated
by `test-builds.sh`) reaches its `make distclean` step. If a unit test
fails, then that cleaning step is not reached, (successful and failed)
unit test logs are preserved and are now added to CI artifacts.
Automated update by ./scripts/update-pot.sh
…#2354) Our TLS Server Hello parser does not treat legacy servers specially, but enabling legacy server support in OpenSSL allows OpenSSL to advance enough in its Server Hello processing to provide our SslBump code with the server certificate (that we then validate). Successful certificate validation, in turn, may result in Squid splicing the connection, even if OpenSSL detected other errors: noteNegotiationError: hold TLS write on FD 15 despite SQUID_TLS_ERR_CONNECT+TLS_LIB_ERR=2000068+TLS_IO_ERR=1 A spliced connection allows the TLS client to handle a legacy server the way the client needs to handle it, without unwanted Squid meddling. This is a Measurement Factory project.
…2347) Use 'dnl' instead of hash to elide the unnecessary blurbs from generated output.
Make sure the stub provides all the symbols declared in fd.h
…e#2364) 2023 commit 4e14397 accidentally removed code that was setting `detailEntry` data member, breaking `%ssl_error_descr` expansion: `Security::ErrorDetail::printErrorDescription()` would always print `[Not available]`. Squid still printed non-configurable request-independent error code _name_ correctly because the corresponding `printErrorCode()` method only uses `detailEntry` as a performance optimization. The effects of this fix are visible, for example, in generated ERR_SECURE_CONNECT_FAIL error responses: ```diff - <p>[Not available]: /CN=...</p> + <p>Certificate does not match domainname: /CN=...</p> ``` This is a Measurement Factory project.
Also moved `fdNFree()` declaration to `fd.h` (to match its definition location) and polished its stub's return value.
Ubuntu Plucky is no longer supported by Canonical, use Questing
…e#2374) In this context, escaping escaped URI always produces incorrect URI because `%` character in the escaped URI gets escaped again. Feeding the result of the first rfc1738_escape() call to the second call is also dangerously wrong because the result of the first call gets invalidated during the second call. No other cases of such "chained" rfc1738_escape() calls were found. Broken since 2002 commit e6ccf24.
…-cache#2373) Squid detects SQUID_X509_V_ERR_DOMAIN_MISMATCH errors during various processing stages, including when receiving an HTTP request on a successfully bumped TLS connection. If that request targets a domain not covered by the server certificate, and sslproxy_cert_error prohibits a mismatch (it does by default), then Squid terminates the transaction with an ERR_SECURE_CONNECT_FAIL response. That generated error response body lacked %x and %D error details: ```diff The system returned: - [No Error] (TLS code: [Unknown Error Code]) + [No Error] (TLS code: SQUID_X509_V_ERR_DOMAIN_MISMATCH) - [No Error Detail] + Certificate does not match domainname: /L=.../O=.../CN=example.com ``` The first `[No Error]` expansion of %E remains unchanged because this particular error does not set `errno`. ConnStateData::serveDelayedError() changes fix the above problem but %x expansion in error pages and %err_detail in access log get a misleading `+broken_cert` detail. To address that flaw, we changed the default for broken certificate in Security::ErrorDetail constructor API from peer certificate to nil. When broken certificate is nil, ErrorDetail now uses valid certificate to expand %ssl_cn and similar certificate-inspecting error page %codes. All Security::ErrorDetail creators were checked and adjusted if needed: * ConnStateData::serveDelayedError(): No caller changes. Using the new ErrorDetail creation API fixes this code by supplying nil broken certificate (because the certificate is _valid_ in this context). * ssl_verify_cb(): No caller changes. We already use peer certificate as the default broken certificate because doing so is "reasonable" here. * Security::PeerConnector::sslCrtvdCheckForErrors(): Adjusted to keep the original "if there was no error_cert_ID, then use peerCert" behavior while using new Security::ErrorDetail creation API. Thus, the last two contexts are not affected by this error reporting API change. The exceptional serveDelayedError() caller is affected, but Squid did not report any certificate detail in that case until this branch fixes, so this branch does not change one "reporting certificate" to another; it only starts reporting (important) information when none was available before. This is a Measurement Factory project.
Fix handling of malformed ICP queries and replies instead of passing invalid URL pointer to consumers, leading to out-of-bounds memory reads and other problems. These fixes affect both ICP v2 and ICP v3 traffic. * Reject packets with URLs that are not NUL-terminated. * Reject packets with URLs containing embedded NULs or trailing garbage. The above two restrictions may backfire if popular ICP agents do send such malformed URLs, and we will need to do more to handle them correctly, but it is _safe_ to reject them for now. Also protect icpHandleUdp() from dereferencing a nil icpOutgoingConn pointer. It is not clear whether icpHandleUdp() can be exposed to nil icpOutgoingConn in current code. More work is needed to polish this.
ACLFilledChecklist correctly locks and unlocks HttpRequest. Thus, when given an unlocked request object, an on-stack checklist destroys it. Upon icpAccessAllowed() return, Squid uses the destroyed request object. This bug was probably introduced in 2003 commit 8000a96 that started automatically unlocking requests in ACLChecklist destructor. However, the bug did not affect allowed ICP v3 queries until 2007 commit f72fb56 started _using_ the request object for them. 2005 commit 319bf5a fixed an equivalent ICP v2 bug for denied queries but missed the ICP v3 case. The scope, age, and effect of this bug imply that Squid v3+ deployments receive no ICP v3 queries since 2007 (or earlier). Squid itself does not send ICP v3 messages, responding with ICP v2 replies to ICP v3 queries. TODO: Consider dropping ICP v3 support. Also moved icpAccessAllowed() inside icpGetRequest() to deduplicate code and reduce the risk of allowing a request without consulting icp_access.
Update Swedish (sv) translations for error pages. Changes: - Removed fuzzy flags from corrected translations - Fixed translation for secure connection error messages - Updated PO-Revision-Date to 2026-02-20 Translator: Daniel Nylander <daniel@danielnylander.se>
The current official GPLv2 text has been modified to remove the FSF street address. Bring our GPL copy up to date with the official GPLv2 license document. Also, libltdl license has similar changes. Update our CREDITS file to match the v2.5.3+ libltdl/ldtl.h import. There are no legal implications from this change.
Cover all FreeBSD versions supported by the FreeBSD Project. Fully rely on packages, not ports, solving the "version mismatch" problem that had led us to remove FreeBSD 13.5 in commit 99fca3a. Enable ccache for faster build times. Restrict testing to the maximus test layer to maximize signal-to-execution-time ratio: The main purpose of FreeBSD tests is to quickly identify Linux-isms and portability issues, not internal code dependencies and misalignments. Prepare to also support arm64 and riscv64, but leave them disabled for now as they are respectively too slow and broken at this time.
SCO OpenServer was EOL'd in 2023. Users wishing to build for this OS can use build time options: ./configure CFLAGS="-lintl"
Co-authored-by: Alex Rousskov <rousskov@measurement-factory.com>
Co-authored-by: Alex Rousskov <rousskov@measurement-factory.com>
8 participants
No description provided.