Skip to content

Conversation

@kit-ty-kate
Copy link
Member

Split from #6751. Will update later

@kit-ty-kate kit-ty-kate added this to the 2.6.0~alpha1 milestone Nov 20, 2025
@kit-ty-kate kit-ty-kate added the PR: WIP Not for merge at this stage label Nov 20, 2025
@dra27
Copy link
Member

dra27 commented Nov 20, 2025

Just parking the last part of #6751 (comment) for convenient re-reading

Without the quoting, I find it more obvious that as the code isn't trying to do anything clever, that it's clearly relying on the fact that quoting isn't needed, and then ensuring that that is true. For example, the names of packages in the src_ext directory (for %.stamp etc.) are expected to come from - I think - [a-zA-Z0-9-]+ which won't ever require escaping in a shell command. If we wanted, I guess we could assert that (needs testing - I haven't actually run this):

QUOTE_SINGLE = '$(subst ','\'',$(1))'
CHECK_TARGET_MATCHES = $(if $(shell echo $(call QUOTE_SINGLE,$(1)) | tr -d '[$(2)]'),\
  echo $(call QUOTE_SINGLE,Target name $(1) doesn't match BRE [$(2)]+); false)

%.stamp:
# Ensure $* can be used unquoted in shell commands
	@$(call CHECK_TARGET_MATCHES,$*,a-zA-Z0-9-)
	...

@dra27
Copy link
Member

dra27 commented Nov 20, 2025

Oo, one other comment I didn't make on the original PR - make has had --warn-undefined-variables for quite some time, but it's difficult to use because it doesn't error. They have recently added other flags for warning on different things and I think being able to surface that as an error (that might not have actually been released yet, though ... I do occasionally hack on GNU make, and I may remembering something from master that isn't released yet).

However, ocaml's CI has some workarounds to be able to test for undefined make variables and make it error in CI; what we've not easily been able to do is get that into a developer's workflow (i.e. have your build fail while you're working on the codebase). The only time it irritates me is having to deal with with testing optional arguments in macros (e.g. having $(call macro,arg1,arg2) but where macro supports an optional $3 - you end up having to wrap it with a check on $(origin) for undefined to avoid the warning).

@kit-ty-kate kit-ty-kate force-pushed the harden-makefile-invariants branch from a68ec34 to 48a5e47 Compare November 24, 2025 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR: WIP Not for merge at this stage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants