Attention is currently required from: Thomas Heijligen.
5 comments:
Patchset:
Great job! It's much better than my initial patch. However, I have a few suggestions.
File Makefile:
Patch Set #1, Line 963: $(if $(findstring $(HAS_SPHINXBUILD),yes), man8/$(PROGRAM).8)
`$(call has_dependency,$(HAS_SPHINXBUILD),man8/$(PROGRAM).8)`
Patch Set #1, Line 1059: $(call install-man)
`$(call has_dependency,$(HAS_SPHINXBUILD),install-man)`
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:
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
```
To view, visit change 74519. To unsubscribe, or for help writing mail filters, visit settings.