Warn when two DOCS entries install to the same filename (issue #115) - #124
jnasbyupgrade wants to merge 3 commits into
Conversation
…ensions#115) - Add check-duplicate-docs phony target that warns about duplicate DOCS entries - Wire it as a prerequisite of all so duplicates surface on any make invocation - Document the failure mode (install refuses to overwrite just-created) and workaround - Document when/why duplicates occur (committed files + generated-file lists) Related changes in pgxntool-test: - Add 3 tests covering the check-duplicate-docs warning (no false positives, catches user-introduced duplicates) Fixes Postgres-Extensions#115. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| # the recipe is empty and make reports "Nothing to be done". | ||
| _PGXNTOOL_DUPLICATE_DOCS = $(strip $(foreach d,$(sort $(DOCS)),\ | ||
| $(if $(word 2,$(filter $(d),$(DOCS))),$(d)))) | ||
|
|
There was a problem hiding this comment.
_PGXNTOOL_DUPLICATE_DOCS compares whole DOCS entries as literal strings via $(filter $(d),$(DOCS)), but the install: will not overwrite just-created failure this check exists to pre-empt is actually keyed on the install destination, i.e. the file's basename — PGXS installs every DOCS entry into a single flat directory ($(INSTALL_DATA) $(addprefix $(srcdir)/, $(DOCS)) '$(DESTDIR)$(docdir)/$(docmoduledir)/').
So two different-path, same-basename entries (e.g. doc/overview.html and extra_doc/overview.html, reachable via the documented DOC_DIRS += extra_doc feature — see base.mk:57/59/64 and README.asc:532-534,598) are not flagged as duplicates here, yet they still hit exactly the will not overwrite just-created failure this PR's own README text describes. The check silently passes while make install still breaks.
Comparing on $(notdir $(DOCS)) instead of the raw entries would close this gap (while still catching the exact-string case the PR's example targets).
There was a problem hiding this comment.
Claude here. Fixed in 3b74c84. The check now groups DOCS entries by $(notdir ...), so two different paths with the same basename count as a collision. The warning names the basename and every source entry, e.g. DOCS installs asc_doc.asc more than once (from: doc/asc_doc.asc extra_doc/asc_doc.asc). README.asc/README.html, HISTORY.asc and the target's comment now describe the check by basename. Postgres-Extensions/pgxntool-test#92 covers this case.
| _PGXNTOOL_DUPLICATE_DOCS = $(strip $(foreach d,$(sort $(DOCS)),\ | ||
| $(if $(word 2,$(filter $(d),$(DOCS))),$(d)))) |
There was a problem hiding this comment.
$(d) is passed as the pattern argument to $(filter $(d),$(DOCS)), but GNU Make's $(filter) treats % in a pattern as a wildcard (matching any sequence of characters), not a literal character — per the GNU Make manual, a literal % must be escaped as \%.
So a DOCS entry containing a literal % produces a false "duplicate" warning against an unrelated file: with DOCS = doc/50%.html doc/500.html (both reachable via the $(wildcard $(dir)/*) populating DOCS at base.mk:59), the pattern doc/50%.html also matches doc/500.html, so $(word 2,...) is non-empty even though doc/50%.html appears only once.
Low severity (warning-only, doesn't fail the build, requires a % in a doc filename), but it's a real false positive from the code as written — e.g. $(filter $(subst %,\%,$(d)),$(DOCS)) would avoid it.
There was a problem hiding this comment.
Claude here. Fixed in 3b74c84. Each entry now goes through $(subst %,\%,...) before it's used as a $(filter) pattern. Postgres-Extensions/pgxntool-test#92 adds a doc/adoc%.adoc entry next to the template's doc/adoc_doc.adoc and asserts that neither is reported. Against the previous base.mk, that same test reports DOCS lists doc/adoc%.adoc more than once.
…es-Extensions#115) PGXS installs every DOCS entry into one flat directory, so two different paths with the same basename collide at install just like a repeated entry. Compare $(notdir) basenames instead of raw entries, and name the colliding source paths in the warning. Escape `%` before using an entry as a $(filter) pattern, so a filename containing `%` can't falsely match another entry. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
jnasbyupgrade
changed the title
1 participant
Summary
Adds a new
check-duplicate-docsphony target that warns when two$(DOCS)entries would install to the same filename. PGXS installs everyDOCSentry into one flat directory, so a repeated entry and two different paths sharing a basename (e.g.doc/foo.htmlandextra_doc/foo.html) both fail the install. The warning names each colliding filename and the entries it came from. The most likely source is an extension's ownDOCS +=line naming a file already picked up by the$(DOC_DIRS)wildcard, which causesmake installto fail with an unhelpful "will not overwrite just-created" error.The warning runs as a prerequisite of
all, so it surfaces on anymake,make install, ormake testinvocation.Paired with Postgres-Extensions/pgxntool-test#92, which adds the test coverage.
Fixes
Fixes #115.
🤖 Generated with Claude Code