April 2023 - flashrom-gerrit - mail.coreboot.org

[M] Change in flashrom[master]: Makefile: Build man-page only when sphinx is available
by Alexander Goncharov (Code Review) 20 Apr '23

20 Apr '23

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 ``` -- To view, visit https://review.coreboot.org/c/flashrom/+/74519 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If1b81d9bc25ecac19d493b44b00a7c42d0643fe6 Gerrit-Change-Number: 74519 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Comment-Date: Thu, 20 Apr 2023 12:01:07 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: Makefile: do not generate man page when building sources
by Anastasia Klimchuk (Code Review) 20 Apr '23

20 Apr '23

Attention is currently required from: Thomas Heijligen, Edward O'Callaghan, Alexander Goncharov. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/74510 ) Change subject: Makefile: do not generate man page when building sources ...................................................................... Patch Set 1: -Code-Review (1 comment) Patchset: PS1: > I've put CB:74519 up, which builds the man-page if sphinx is available. […] Yes, this seems like a more proper solution. Joursoir, I think we need to go ahead with CB:74519 , I hope you are not hurt! I really appreciate you wanted to solve the issue, thank you! -- To view, visit https://review.coreboot.org/c/flashrom/+/74510 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9882445de2a0996f237470a73b6a1eb7eafce808 Gerrit-Change-Number: 74510 Gerrit-PatchSet: 1 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Thu, 20 Apr 2023 11:47:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.de> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: Makefile: Build man-page only when sphinx is available
by Anastasia Klimchuk (Code Review) 20 Apr '23

20 Apr '23

Attention is currently required from: Thomas Heijligen, Alexander Goncharov. Anastasia Klimchuk 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: Code-Review+1 (2 comments) File Makefile: https://review.coreboot.org/c/flashrom/+/74519/comment/b02dd93c_e2cc0e6c PS1, Line 1024: @echo "Found \"sphinx-build\" to generate man-pages: $(HAS_SPHINXBUILD)" The previous lines have the pattern of message "Checking for XYZ: yes/no" maybe lets follow the pattern, Checking for \"sphinx-build\" to generate man-pages: $(HAS_SPHINXBUILD) ? https://review.coreboot.org/c/flashrom/+/74519/comment/9639cfb2_1e3e87d0 PS1, Line 1036: man8/$(PROGRAM).8: When is this executed? which command runs this? -- To view, visit https://review.coreboot.org/c/flashrom/+/74519 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If1b81d9bc25ecac19d493b44b00a7c42d0643fe6 Gerrit-Change-Number: 74519 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Thu, 20 Apr 2023 11:45:40 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: usb_device.c: remove LIBUSB() wrapper around call that may fail
by Edward O'Callaghan (Code Review) 20 Apr '23

20 Apr '23

Attention is currently required from: Nikolai Artemiev. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/74537 ) Change subject: usb_device.c: remove LIBUSB() wrapper around call that may fail ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/74537 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I38e4642bcbddaf3f37821093f6b919806134ed7b Gerrit-Change-Number: 74537 Gerrit-PatchSet: 1 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Thu, 20 Apr 2023 08:09:38 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: meson.build: Use library() to build libflashrom
by Edward O'Callaghan (Code Review) 20 Apr '23

20 Apr '23

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/73101 ) ( 5 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: meson.build: Use library() to build libflashrom ...................................................................... meson.build: Use library() to build libflashrom Use library() instead of both_libraries() to build libflashrom. The built-in option `default-library` can be used to decide which kind of libraries should be built. Make `both` the default and throw an error in the case someone tries to build the classic_cli with a shared library. Change-Id: I27f10fdf1227795a9a3b4e050a2d708b58f10ee7 Signed-off-by: Thomas Heijligen <thomas.heijligen(a)secunet.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/73101 Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M meson.build 1 file changed, 31 insertions(+), 3 deletions(-) Approvals: build bot (Jenkins): Verified Edward O'Callaghan: Looks good to me, approved Anastasia Klimchuk: Looks good to me, approved diff --git a/meson.build b/meson.build index 5c38749..4341ccf 100644 --- a/meson.build +++ b/meson.build @@ -8,9 +8,17 @@ 'werror=true', 'optimization=s', 'debug=false', + 'default_library=both' ], ) +if get_option('classic_cli').enabled() and get_option('default_library') == 'shared' + error(''' + Cannot build cli_classic with shared libflashrom. Use \'-Dclassic_cli=disabled\' to disable the cli, + or use \'--default-library=both\' to also build the classic_cli + ''') +endif + subdir('doc') # libtool versioning @@ -587,7 +595,7 @@ else vflag = '-Wl,--version-script,@0@/@1@'.format(meson.current_source_dir(), mapfile) endif -libflashrom = both_libraries( +libflashrom = library( 'flashrom', sources : [ srcs, @@ -624,7 +632,7 @@ description : 'library to interact with flashrom', ) -if get_option('classic_cli').auto() or get_option('classic_cli').enabled() +if get_option('classic_cli').enabled() or get_option('classic_cli').auto() and not get_option('default_library') == 'shared' classic_cli = executable( 'flashrom', files( @@ -637,7 +645,8 @@ install : true, install_dir : get_option('sbindir'), link_args : link_args, - link_with : libflashrom.get_static_lib(), # flashrom needs internal symbols of libflashrom + # flashrom needs internal symbols of libflashrom + link_with : get_option('default_library') == 'static' ? libflashrom : libflashrom.get_static_lib(), ) if get_option('llvm_cov').enabled() run_target('llvm-cov-cli', command : ['scripts/llvm-cov', classic_cli]) -- To view, visit https://review.coreboot.org/c/flashrom/+/73101 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I27f10fdf1227795a9a3b4e050a2d708b58f10ee7 Gerrit-Change-Number: 73101 Gerrit-PatchSet: 8 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[S] Change in flashrom[master]: tests: Fix mode_t argument conversion for va_arg
by Thomas Heijligen (Code Review) 20 Apr '23

20 Apr '23

Attention is currently required from: Anastasia Klimchuk. Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/74202 ) Change subject: tests: Fix mode_t argument conversion for va_arg ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/74202 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I95b7c8dafdf4e7664c48a952acd7f8eaedb59ba7 Gerrit-Change-Number: 74202 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Idwer Vollering <vidwer(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Thu, 20 Apr 2023 06:52:14 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: doc: Add contact page
by Edward O'Callaghan (Code Review) 20 Apr '23

20 Apr '23

Attention is currently required from: Thomas Heijligen, Alexander Goncharov. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/73822 ) Change subject: doc: Add contact page ...................................................................... Patch Set 6: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/73822 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ibfba6a59c5a945b4238d16e07a07584f94159568 Gerrit-Change-Number: 73822 Gerrit-PatchSet: 6 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Thu, 20 Apr 2023 04:00:09 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: tests: Fix mode_t argument conversion for va_arg
by Thomas Heijligen (Code Review) 19 Apr '23

19 Apr '23

Attention is currently required from: Anastasia Klimchuk. Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/74202 ) Change subject: tests: Fix mode_t argument conversion for va_arg ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: The same issue appears on MacOS -- To view, visit https://review.coreboot.org/c/flashrom/+/74202 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I95b7c8dafdf4e7664c48a952acd7f8eaedb59ba7 Gerrit-Change-Number: 74202 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Idwer Vollering <vidwer(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Wed, 19 Apr 2023 15:11:22 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: Makefile: Use printf instead of echo -n
by Paul Menzel (Code Review) 19 Apr '23

19 Apr '23

Attention is currently required from: Thomas Heijligen. Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/74522 ) Change subject: Makefile: Use printf instead of echo -n ...................................................................... Patch Set 1: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/74522 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I96bb6c60e74133bbc86d5069cb076ee43f4bca83 Gerrit-Change-Number: 74522 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Comment-Date: Wed, 19 Apr 2023 14:26:17 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: Makefile: Use printf instead of echo -n
by Thomas Heijligen (Code Review) 19 Apr '23

19 Apr '23

Thomas Heijligen has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/74522 ) Change subject: Makefile: Use printf instead of echo -n ...................................................................... Makefile: Use printf instead of echo -n On MacOS `echo -n` is not working. Change-Id: I96bb6c60e74133bbc86d5069cb076ee43f4bca83 Signed-off-by: Thomas Heijligen <thomas.heijligen(a)secunet.com> --- M Makefile 1 file changed, 13 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/22/74522/1 diff --git a/Makefile b/Makefile index ec79963..bdf281d 100644 --- a/Makefile +++ b/Makefile @@ -967,7 +967,7 @@ config: @echo Building flashrom version $(VERSION) - @echo -n "C compiler found: " + @printf "C compiler found: " @if [ $(CC_WORKING) = yes ]; \ then $(CC) --version 2>/dev/null | head -1; \ else echo no; echo Aborting.; exit 1; fi -- To view, visit https://review.coreboot.org/c/flashrom/+/74522 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I96bb6c60e74133bbc86d5069cb076ee43f4bca83 Gerrit-Change-Number: 74522 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-MessageType: newchange

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit April 2023