Jeff King wrote:

On Mon, May 17, 2021 at 05:53:25AM -0500, Felipe Contreras wrote:

quoted

quoted

It's meant for the caller of "make". Your proposed use is within
doc-diff, but any user running "make ASCIIDOC_EXTRA=foo" would see the
different behavior.

Yeah, they would, but I don't think it would be wrong behavior.

It depends what they're trying to do. If they write:

  make ASCIIDOC_EXTRA=--one-extra-option

then they probably intend to to add to the options we set. If they
write:

  make ASCIIDOC_EXTRA='-acompat-mode -atabsize=4 ...etc...'

with the intent of replicating the flags but changing or removing some
elements, then it would no longer do what they want.

I don't mean to imply one is more right than the other (I'd suspect even
that the override behavior is more likely to be what somebody wants).

Yeah, but I am implying that one is more right than the other.

I'm mostly pointing out that this is unlike the rest of our Makefiles,
which do not ever use override (and that the effect is visible to the
caller, depending on what they want to do).

It's used in the main Makefile, although in a different way.

I see how it is not consistent with the rest of the Makefiles, but I
wonder why it's not being used. It's rather useful.

quoted

quoted

I'd probably call it ASCIIDOC_FLAGS (like we have CFLAGS and LDFLAGS
that are meant for users to inform us of extra flags they'd like
passed).

Right, but Makefiles do override those, like:

  override CFLAGS += -fPIC

Otherwise builds may fail.

Some Makefiles do, but in this project we have not historically used
override. Instead, we provide defaults for things like CFLAGS, expect
the use to replace them if they like, and then aggregate them (along
with other internal variables) into things like ALL_CFLAGS.

I know, but status quo is not an argument.

If we always did things the way we've always done things there would
never be progress.

I'm aruging there's no value in giving the user the opportunity to break
the build by doing `make BASIC_CFLAGS=`. Yes, it's more historically
consistent, that doesn't mean it's good.

quoted

quoted

Of course that may not solve your problem in a sense; if you want
doc-diff to override it, then that might conflict with a theoretical
ASCIIDOC_FLAGS somebody set in their config.mak (but we really are in
the realm of hypothetical here).

Setting ASCIIDOC_FLAGS in config.mk would not override the
user-supplied flags any more than setting them in the Makefile (they are
virtually the same thing as one includes the other).

It's only if the user has `override ASCIIDOC_FLAGS` in config.mk that
such a problem would arise. And that's really hypothetical.

I mean that if your doc-diff runs:

  make USE_ASCIIDOCTOR=Yes ASCIIDOC_FLAGS=-adocdate=01/01/1970

then that will override anything the user put into config.mak. If they
had some option like:

  ASCIIDOC_FLAGS = --load-path=/some/special/directory

they need for asciidoctor to run correctly on their system, then things
would break for them. But we don't even have a user-facing
ASCIIDOC_FLAGS now, and nobody is asking for it, so it's pretty
hypothetical (I'd guess somebody in this situation would just set
ASCIIDOC="asciidoctor --load-path=...", and that already doesn't work
with doc-diff).

Exactly, so it's unclear how much value we get by talking about these.

Either way, I don't feel very strongly about `override ASCIIDOC_EXTRA`.
I think it's superior but ASCIIDOC_FLAGS requires less changes, so I'm
fine with that.

Cheers.

-- 
Felipe Contreras