opam icon indicating copy to clipboard operation
opam copied to clipboard

Harden makefile invariants

Open kit-ty-kate opened this issue 3 months ago • 2 comments

Split from https://github.com/ocaml/opam/pull/6751. Will update later

kit-ty-kate avatar Nov 20 '25 11:11 kit-ty-kate

Just parking the last part of https://github.com/ocaml/opam/pull/6751#issuecomment-3557469872 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 avatar Nov 20 '25 11:11 dra27

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).

dra27 avatar Nov 20 '25 11:11 dra27