| Message ID | 20210514121435.504423-2-felipe.contreras@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | doc: asciidoctor: direct man page creation and fixes | expand |
On Fri, May 14, 2021 at 07:14:25AM -0500, Felipe Contreras wrote:
> Without `override` all additions will be ignored by make.
That's true of all of our Makefile variables. Is there a particular
reason to give this one special treatment?
-Peff
On Sat, May 15, 2021 at 05:32:08AM -0400, Jeff King wrote: > On Fri, May 14, 2021 at 07:14:25AM -0500, Felipe Contreras wrote: > > > Without `override` all additions will be ignored by make. > > That's true of all of our Makefile variables. Is there a particular > reason to give this one special treatment? To go into further detail: usually we distinguish variables we use internally from user-facing ones, and include the latter in the former. I see a later patch wants to start passing ASCIIDOC_EXTRA on the command-line, and we'd use two variables for that. -Peff
Jeff King wrote: > On Sat, May 15, 2021 at 05:32:08AM -0400, Jeff King wrote: > > > On Fri, May 14, 2021 at 07:14:25AM -0500, Felipe Contreras wrote: > > > > > Without `override` all additions will be ignored by make. > > > > That's true of all of our Makefile variables. Is there a particular > > reason to give this one special treatment? > > To go into further detail: usually we distinguish variables we use > internally from user-facing ones, and include the latter in the former. > I see a later patch wants to start passing ASCIIDOC_EXTRA on the > command-line, and we'd use two variables for that. Well, it's not exactly user-facing; it's only needed for doc-diff. Would TEST_ASCIIDOC_EXTRA make sense?
On Sat, May 15, 2021 at 07:13:48AM -0500, Felipe Contreras wrote: > Jeff King wrote: > > On Sat, May 15, 2021 at 05:32:08AM -0400, Jeff King wrote: > > > > > On Fri, May 14, 2021 at 07:14:25AM -0500, Felipe Contreras wrote: > > > > > > > Without `override` all additions will be ignored by make. > > > > > > That's true of all of our Makefile variables. Is there a particular > > > reason to give this one special treatment? > > > > To go into further detail: usually we distinguish variables we use > > internally from user-facing ones, and include the latter in the former. > > I see a later patch wants to start passing ASCIIDOC_EXTRA on the > > command-line, and we'd use two variables for that. > > Well, it's not exactly user-facing; it's only needed for doc-diff. 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. > Would TEST_ASCIIDOC_EXTRA make sense? 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). 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). -Peff
Jeff King wrote: > On Sat, May 15, 2021 at 07:13:48AM -0500, Felipe Contreras wrote: > > Jeff King wrote: > > > To go into further detail: usually we distinguish variables we use > > > internally from user-facing ones, and include the latter in the former. > > > I see a later patch wants to start passing ASCIIDOC_EXTRA on the > > > command-line, and we'd use two variables for that. > > > > Well, it's not exactly user-facing; it's only needed for doc-diff. > > 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. > > Would TEST_ASCIIDOC_EXTRA make sense? > > 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. > 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. Cheers.
On Mon, May 17, 2021 at 05:53:25AM -0500, Felipe Contreras wrote: > > 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). 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). > > 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. > > 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). -Peff
Jeff King wrote: > On Mon, May 17, 2021 at 05:53:25AM -0500, Felipe Contreras wrote: > > > > 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. > > > 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. > > > 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.
diff --git a/Documentation/Makefile b/Documentation/Makefile index f5605b7767..981e322f18 100644 --- a/Documentation/Makefile +++ b/Documentation/Makefile @@ -191,9 +191,9 @@ ASCIIDOC = asciidoctor ASCIIDOC_CONF = ASCIIDOC_HTML = xhtml5 ASCIIDOC_DOCBOOK = docbook5 -ASCIIDOC_EXTRA += -acompat-mode -atabsize=8 -ASCIIDOC_EXTRA += -I. -rasciidoctor-extensions -ASCIIDOC_EXTRA += -alitdd='&\#x2d;&\#x2d;' +override ASCIIDOC_EXTRA += -acompat-mode -atabsize=8 +override ASCIIDOC_EXTRA += -I. -rasciidoctor-extensions +override ASCIIDOC_EXTRA += -alitdd='&\#x2d;&\#x2d;' ASCIIDOC_DEPS = asciidoctor-extensions.rb GIT-ASCIIDOCFLAGS DBLATEX_COMMON = XMLTO_EXTRA += --skip-validation @@ -206,12 +206,12 @@ SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH)) ifdef DEFAULT_PAGER DEFAULT_PAGER_SQ = $(subst ','\'',$(DEFAULT_PAGER)) -ASCIIDOC_EXTRA += -a 'git-default-pager=$(DEFAULT_PAGER_SQ)' +override ASCIIDOC_EXTRA += -a 'git-default-pager=$(DEFAULT_PAGER_SQ)' endif ifdef DEFAULT_EDITOR DEFAULT_EDITOR_SQ = $(subst ','\'',$(DEFAULT_EDITOR)) -ASCIIDOC_EXTRA += -a 'git-default-editor=$(DEFAULT_EDITOR_SQ)' +override ASCIIDOC_EXTRA += -a 'git-default-editor=$(DEFAULT_EDITOR_SQ)' endif QUIET_SUBDIR0 = +$(MAKE) -C # space to separate -C and subdir @@ -375,7 +375,7 @@ technical/api-index.txt: technical/api-index-skel.txt \ technical/api-index.sh $(patsubst %,%.txt,$(API_DOCS)) $(QUIET_GEN)cd technical && '$(SHELL_PATH_SQ)' ./api-index.sh -technical/%.html: ASCIIDOC_EXTRA += -a git-relative-html-prefix=../ +technical/%.html: override ASCIIDOC_EXTRA += -a git-relative-html-prefix=../ $(patsubst %,%.html,$(API_DOCS) technical/api-index $(TECH_DOCS)): %.html : %.txt \ asciidoc.conf GIT-ASCIIDOCFLAGS $(QUIET_ASCIIDOC)$(TXT_TO_HTML) $*.txt @@ -425,7 +425,7 @@ $(patsubst %,%.html,$(ARTICLES)) : %.html : %.txt WEBDOC_DEST = /pub/software/scm/git/docs -howto/%.html: ASCIIDOC_EXTRA += -a git-relative-html-prefix=../ +howto/%.html: override ASCIIDOC_EXTRA += -a git-relative-html-prefix=../ $(patsubst %.txt,%.html,$(HOWTO_TXT)): %.html : %.txt GIT-ASCIIDOCFLAGS $(QUIET_ASCIIDOC) \ sed -e '1,/^$$/d' $< | \
Without `override` all additions will be ignored by make. Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> --- Documentation/Makefile | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)