Fix missing release notes#2413
Conversation
The linuxdoc tool used to generate release notes is not optional.
|
This problem has been around for a while, preventing certain types of patching by downstream vendors. |
This comment was marked as resolved.
This comment was marked as resolved.
|
apart from this, LGTM |
That is to ensure that tools identified by ./configure (eg when cross-compiling) are used, not anything located in the shell PATH. Updated the description to say that. FWIW, I hit the above problem when testing the change to |
I agree it's valuable, no question.
I'm not going to ask you to put in the work to separate them, but I do ask you to highlight there's two changes |
rousskov
left a comment
There was a problem hiding this comment.
PR description: Vendors typically use autoreconf to rebuilds Squid ... That tool performs an equivalent to 'make distclean' which erases RELEASENOTES.html.
That (true) statement feels misleading in this PR context because, AFAICT, even make clean erases RELEASENOTES.html (in both official and PR sources): "Vendors" and "autoreconf" could be the motivation for this PR, of course, but the way they are currently mentioned is a red herring -- regular users rebuilding Squid under regular conditions erase RELEASENOTES.html as well. Please rephrase PR description to address this concern.
PR description: Ensuring that file is always able to be re-created if missing.
It is difficult for me to understand what this PR has changed at the top level (other than introducing a linuxdoc availability as make dist success precondition, and even that conclusion requires some guessing!).
Please rephrase PR title and description to be more specific, highlighting the top-level/primary change to RELEASENOTES.html creation targets. For example, "Prior to this changes, make ... created RELEASENOTES.html. Now, make dist creates it." Same for the primary target that erases that generated file (if that target has changed).
| -e "s%[@]SQUID_RELEASE[@]%$(SQUID_RELEASE)%g" \ | ||
| -e "s%[@]SQUID_RELEASE_OLD[@]%$$(( $(SQUID_RELEASE) - 1 ))%g" \ | ||
| < $< >$@ | ||
| test `grep -c "@SQUID" $@` -eq 0 |
There was a problem hiding this comment.
BTW, this target and/or this particular test is broken: This target should fail when $@ does not exist but it actually succeeds. Since this PR removes ENABLE_RELEASE_DOCS protections -- exposing this code to more build cases/environments, it may be a good idea to fix this in this PR, but I do not insist on that.
There was a problem hiding this comment.
"Works for me". Dropping a rm $@ command after the sed to replicate a failure to produce output I get this:
/usr/bin/sed \
-e "s%[@]SQUID_VERSION[@]%8.0.0-VCS%g" \
-e "s%[@]SQUID_RELEASE[@]%8%g" \
-e "s%[@]SQUID_RELEASE_OLD[@]%$(( 8 - 1 ))%g" \
< ../../../doc/release-notes/release-8.sgml.in >release-8.sgml ; rm -f release-8.sgml
test `grep -c "@SQUID" release-8.sgml` -eq 0
grep: release-8.sgml: No such file or directory
/bin/bash: line 1: test: -eq: unary operator expected
Complain about the command, not about the inability to generate the output.
The move to
At the top level the dependency chain for
Not as easy as that. It is more work than I am willing to invest just for a commit message, to track down all the possible targets that lead to the multiple old
Done. Only the way to create it has changed now. |
Vendors typically use autoreconf to rebuild Squid after patching
the Makefile.am or configure.ac sources. That tool performs an
equivalent to 'make distclean' which erases RELEASENOTES.html.
Refactor the RELEASENOTES.html creation using automake rules
with transitive dependency and clean support, and automake
variables for tools detected by ./configure are used for the
build. Ensuring that file is always able to be re-created if
missing via 'make dist' or 'make RELEASENOTES.html' commands.
The need to re-generate RELEASENOTES.html adds the linuxdoc tool
to build dependencies where it was previously only mandatory for
release maintainer to generate the "Bootstrapped sources"
tarball.