Skip to content

autotools: minor fixes and improvements#22154

Closed
vszakats wants to merge 11 commits into
curl:masterfrom
vszakats:amfixes
Closed

autotools: minor fixes and improvements#22154
vszakats wants to merge 11 commits into
curl:masterfrom
vszakats:amfixes

Conversation

@vszakats

@vszakats vszakats commented Jun 24, 2026

Copy link
Copy Markdown
Member

Cherry-picked from #22102
Cherry-picked from #22150

vszakats added 4 commits June 24, 2026 13:43
… commands

And to `sha256sum`, where missing.

Also:
- maketgz: pass args with `:?` to `rm -rf`, where missing.
- tests/Makefile.am: drop unused remains of `MANFILES`.
  Follow-up to fa3f889 curl#17463
- tests/Makefile.am: fix to dist-clean `CLEANFILES`. [WIP]
- tests/http/Makefile.am: add `./` prefix to glob passed to `rm -rf`.

Follow-up to 6aab1dc curl#19450

Closes curl#22150

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR refines the Autotools build/test Makefile targets and clean/dist behavior to better mirror existing CMake conveniences and to align generated-file cleanup with Automake expectations.

Changes:

  • Adds top-level tests and test-quiet targets and adjusts top-level test mapping.
  • Updates dist-hook file copying to include an end-of-options marker for cp.
  • Cleans up tests-related Makefile logic (drop old MANFILES remnants; adjust cleanup of generated artifacts).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
Makefile.am Adds/adjusts top-level test targets and tweaks dist-hook copy invocation
tests/Makefile.am Removes MANFILES linkage and changes distclean cleanup behavior
tests/certs/Makefile.am Switches generated cert cleanup from DISTCLEANFILES to CLEANFILES
tests/http/Makefile.am Makes rm -rf glob safer by prefixing with ./

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread Makefile.am
Comment thread tests/Makefile.am
Comment on lines 184 to +185
distclean:
rm -f $(MANFILES)
rm -f $(CLEANFILES)

@vszakats vszakats Jun 24, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Perhaps the LLM has just as little input on how this works as humans do...

I tested the LLM's theory and it failed CI:
https://github.com/curl/curl/actions/runs/28100635953/job/83201418475?pr=22154
so I conclude this is inaccurate.

Sadly the manual does not help to get a better understanding.
It must be the "heuristics".

vszakats added 5 commits June 24, 2026 14:29
Ref: curl#22154 (comment)
"Defining a custom `distclean:` target in this Makefile overrides
Automake’s default recursive `distclean` logic, which is especially
problematic here because `SUBDIRS` is set. Use `distclean-local:`
instead so the standard distclean behavior (including subdir recursion)
is preserved."
@curl curl deleted a comment from superagent-security Bot Jun 24, 2026
@dfandrich

dfandrich commented Jun 24, 2026 via email

Copy link
Copy Markdown
Contributor

@vszakats

Copy link
Copy Markdown
Member Author

• Makefile.am: use end-of-options marker for cp command in the dist-hook target. It's not POSIX syntax, but complements the previous similar fix made for the same target.
I don't like how we're breaking normal installation for some users, especially since we know that none of the files in the curl repo have problematic names. The other way to deal with this theoretical problem is ensuring that the source name is either absolute (and therefore starts with /) or relative by prefixing with ./

Indeed, I've since deleted this update from #22150. This PR message
entry was also wrong for this PR, which didn't do this change, nor did
it intend to. Thanks for spotting it!

(as an aside the pre-existing -- use remains. I'm not sure how we could
avoid that: find $(distdir) -name "*.dist" -exec rm -- {} \;

@bagder

bagder commented Jun 25, 2026

Copy link
Copy Markdown
Member

I'm not sure how we could avoid that: find $(distdir) -name "*.dist" -exec rm -- {} ;

That command invoke is for making release tarballs, so if we want we can just drop the -- from there and it won't hurt anyone. and it seems rather paranoid to expect one if the *.dist files in a release scenario to do something terrible. Not to mention that we run this command in a dedicated docker container nowadays so we would detect if it breaks.

But also. We do release tarballs on a limited set of machines and they have -- support so its also easy to just keep this there.

@vszakats vszakats closed this in 2f4d322 Jun 25, 2026
@vszakats
vszakats deleted the amfixes branch June 25, 2026 09:36
@vszakats

Copy link
Copy Markdown
Member Author

Let's leave it there then, I'd say, also agreeing it's paranoid (as many updates these days!).

@dfandrich

dfandrich commented Jun 25, 2026 via email

Copy link
Copy Markdown
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

4 participants