Attention is currently required from: Thomas Heijligen.
Alexander Goncharov has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/74519 )
Change subject: Makefile: Build man-page only when sphinx is available ......................................................................
Patch Set 1:
(5 comments)
Patchset:
PS1: Great job! It's much better than my initial patch. However, I have a few suggestions.
File Makefile:
https://review.coreboot.org/c/flashrom/+/74519/comment/f12cfbcc_ca89f32e PS1, Line 963: $(if $(findstring $(HAS_SPHINXBUILD),yes), man8/$(PROGRAM).8) `$(call has_dependency,$(HAS_SPHINXBUILD),man8/$(PROGRAM).8)`
https://review.coreboot.org/c/flashrom/+/74519/comment/0b309405_825739e7 PS1, Line 1059: $(call install-man) `$(call has_dependency,$(HAS_SPHINXBUILD),install-man)`
https://review.coreboot.org/c/flashrom/+/74519/comment/9c91c4d5_e77556ea PS1, Line 1058: : install: install-bin install-bash $(call install-man) : : install-bin: $(PROGRAM)$(EXEC_SUFFIX) : mkdir -p $(DESTDIR)$(PREFIX)/sbin : $(INSTALL) -m 0755 $(PROGRAM)$(EXEC_SUFFIX) $(DESTDIR)$(PREFIX)/sbin : : install-bash: $(PROGRAM).bash : mkdir -p $(DESTDIR)$(BASHCOMPDIR) : $(INSTALL) -m 0644 $(PROGRAM).bash $(DESTDIR)$(BASHCOMPDIR) : : install-man: man8/$(PROGRAM).8 : mkdir -p $(DESTDIR)$(MANDIR)/man8 : $(INSTALL) -m 0644 man8/$(PROGRAM).8 $(DESTDIR)$(MANDIR)/man8 : : install-lib: libflashrom.a include/libflashrom.h : mkdir -p $(DESTDIR)$(PREFIX)/lib : $(INSTALL) -m 0644 libflashrom.a $(DESTDIR)$(PREFIX)/lib : mkdir -p $(DESTDIR)$(PREFIX)/include : $(INSTALL) -m 0644 include/libflashrom.h $(DESTDIR)$(PREFIX)/include : : libinstall: install-lib How about doing these changes as a separate patch? Because it looks like a refactoring and not an essential part of the commit.
In the separate patch it would be `install: install-bin install-bash install-man`. And in this patch you change `install-man` to `$(call install-man)`.
It will make the git history cleaner :)
File Makefile.include:
https://review.coreboot.org/c/flashrom/+/74519/comment/6b5d5e5e_8e1c8be3 PS1, Line 54: : define install-man : $(if $(findstring $(HAS_SPHINXBUILD),yes), install-man) : endef You can make this macro/fun more universal, e.g:
``` define has_dependency # $1: dependency, $2: action/target $(if $(findstring $1,yes), $2) endef ```