Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Makefile.include: only warn if not curl, wget, unzip, 7z #16784

Merged
merged 1 commit into from
Sep 2, 2021
Merged

Makefile.include: only warn if not curl, wget, unzip, 7z #16784

merged 1 commit into from
Sep 2, 2021

Conversation

fjmolinas
Copy link
Contributor

Contribution description

Currently Makefile.include errors if these tools are absent, but they are actually rarely used, so as all other tools used by only some packages don't error, but keep the warning.

$ git grep \$\(CURL\)
Makefile.include:  ifeq (,$(CURL))
Makefile.include:  ifeq (,$(WGET)$(CURL))
Makefile.include:    DOWNLOAD_TO_STDOUT ?= $(if $(CURL),$(CURL) -s,$(WGET) -q -O-)
Makefile.include:    DOWNLOAD_TO_FILE ?= $(if $(WGET),$(WGET) -nv -c -O,$(CURL) -s -o)

$ git grep \$\(WGET\)
Makefile.include:  ifeq (,$(WGET))
Makefile.include:  ifeq (,$(WGET)$(CURL))
Makefile.include:    DOWNLOAD_TO_STDOUT ?= $(if $(CURL),$(CURL) -s,$(WGET) -q -O-)
Makefile.include:    DOWNLOAD_TO_FILE ?= $(if $(WGET),$(WGET) -nv -c -O,$(CURL) -s -o)

$ git grep \$\(DOWNLOAD_TO_STDOUT\)
Makefile.include:ifeq (,$(and $(DOWNLOAD_TO_STDOUT),$(DOWNLOAD_TO_FILE)))
Makefile.include:  ifeq (,$(DOWNLOAD_TO_STDOUT))
makefiles/info.inc.mk:  @echo 'DOWNLOAD_TO_STDOUT: $(DOWNLOAD_TO_STDOUT)'
makefiles/vars.inc.mk:export DOWNLOAD_TO_STDOUT    # Use `$(DOWNLOAD_TO_STDOUT) $(URL)` to download `$(URL)` output `$(URL)` to stdout, e.g. to be piped into `tar xz`.

$ git grep \$\(DOWNLOAD_TO_FILE\)
Makefile.include:ifeq (,$(and $(DOWNLOAD_TO_STDOUT),$(DOWNLOAD_TO_FILE)))
Makefile.include:  ifeq (,$(DOWNLOAD_TO_FILE))
makefiles/info.inc.mk:  @echo 'DOWNLOAD_TO_FILE:   $(DOWNLOAD_TO_FILE)'
makefiles/vars.inc.mk:export DOWNLOAD_TO_FILE      # Use `$(DOWNLOAD_TO_FILE) $(DESTINATION) $(URL)` to download `$(URL)` to `$(DESTINATION)`.
pkg/Makefile.http:      $(Q)$(DOWNLOAD_TO_FILE) $@ $(PKG_URL)/$(PKG_NAME)-$(PKG_VERSION).$(PKG_EXT)
pkg/c25519/Makefile:    $(Q)$(DOWNLOAD_TO_FILE) $@ $(PKG_URL)/$(PKG_NAME)-$(PKG_VERSION).$(PKG_EXT)

$ git grep \$\(UNZIP_HERE\)
Makefile.include:ifeq (,$(UNZIP_HERE))
makefiles/info.inc.mk:  @echo 'UNZIP_HERE:         $(UNZIP_HERE)'
makefiles/vars.inc.mk:export UNZIP_HERE            # Use `cd $(SOME_FOLDER) && $(UNZIP_HERE) $(SOME_FILE)` to extract the contents of the zip file `$(SOME_FILE)` into `$(SOME_FOLDER)`.
pkg/Makefile.http:      $(Q)$(UNZIP_HERE) $<
pkg/c25519/Makefile:    $(Q)$(UNZIP_HERE) -D -d $(PKGDIRBASE) $<

It also uses memoized do not call the shell functions if not needed, but still works if they are required:

Testing procedure

  • green murdock
make -C tests/pkg_c25519/ distclean clean all
rm -rf /home/francisco/workspace/RIOT/build/pkg/c25519 /home/francisco/workspace/RIOT/build/pkg/c25519-2017-10-05.zip
# Reset package to checkout state.
rm -rf /home/francisco/workspace/RIOT/build/pkg/c25519
Building application "tests_pkg_c25519" for "native" with MCU "native".

mkdir -p /home/francisco/workspace/RIOT/build/pkg
2021-08-28 11:39:47 URL:https://www.dlbeer.co.nz/downloads/c25519-2017-10-05.zip [68419/68419] -> "/home/francisco/workspace/RIOT/build/pkg/c25519-2017-10-05.zip" [1]
test "dbfb4285837ab2ea3d99c448b22877cc7a139ccbaebb1de367e2bec1fd562fe629b389d86603915448078b8fd7e631c8fc9a7d126eb889a1ba0c17611369b190  /home/francisco/workspace/RIOT/build/pkg/c25519-2017-10-05.zip" =  "$(sha512sum "/home/francisco/workspace/RIOT/build/pkg/c25519-2017-10-05.zip")"
"make" -C /home/francisco/workspace/RIOT/pkg/c25519
"make" -C /home/francisco/workspace/RIOT/build/pkg/c25519/src -f /home/francisco/workspace/RIOT/Makefile.base MODULE=c25519
"make" -C /home/francisco/workspace/RIOT/boards/native
"make" -C /home/francisco/workspace/RIOT/boards/native/drivers
"make" -C /home/francisco/workspace/RIOT/core
"make" -C /home/francisco/workspace/RIOT/cpu/native
"make" -C /home/francisco/workspace/RIOT/cpu/native/periph
"make" -C /home/francisco/workspace/RIOT/cpu/native/stdio_native
"make" -C /home/francisco/workspace/RIOT/drivers
"make" -C /home/francisco/workspace/RIOT/drivers/periph_common
"make" -C /home/francisco/workspace/RIOT/sys
"make" -C /home/francisco/workspace/RIOT/sys/auto_init
"make" -C /home/francisco/workspace/RIOT/sys/embunit
"make" -C /home/francisco/workspace/RIOT/sys/luid
"make" -C /home/francisco/workspace/RIOT/sys/random
"make" -C /home/francisco/workspace/RIOT/sys/random/tinymt32
"make" -C /home/francisco/workspace/RIOT/sys/test_utils/interactive_sync
   text	   data	    bss	    dec	    hex	filename
  51078	    716	  64364	 116158	  1c5be	/home/francisco/workspace/RIOT/tests/pkg_c25519/bin/native/tests_pkg_c25519.elf

Issues/PRs references

Follow up to #16776

@fjmolinas fjmolinas added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 28, 2021
@fjmolinas fjmolinas requested review from chrysn and miri64 August 28, 2021 09:42
@github-actions github-actions bot added the Area: build system Area: Build system label Aug 28, 2021
@fjmolinas fjmolinas mentioned this pull request Aug 28, 2021
Merged
@fjmolinas
Makefile.include: warning not error if not curl, wget, unzip, 7z
fee3169 
The following tools are not needed for every build, its only needed
for a couple of packages like c25519, since most are fetched with git.
@fjmolinas fjmolinas force-pushed the pr_curl_wget_unzip_7z_no_error branch from 0392dff to fee3169 Compare August 28, 2021 09:56
@chrysn
Copy link
Member

chrysn commented Aug 28, 2021 via email

@fjmolinas
Copy link
Contributor Author

Just to check for use cases: has anyone ever encountered a RIOT-usable system with no curl or wget and no unzipper? (I've only ever seen these errors when which or command is broken).

I've had them when using minimal Docker images for building specific applications, or for flashing. e.g.: flashing with openocd, I'm forced to inlcude wget and unzip in the image just because of this.

@chrysn
Copy link
Member

chrysn commented Aug 29, 2021

Another question to understand the patch: Why run through the bespoke memoized rather than just assigning immediately? I can't think of it as an optimization (especially because during assignment time the which binary and relevant paths are bound to be fresh in the OS cache), and don't see any other effect.

@fjmolinas
Copy link
Contributor Author

Another question to understand the patch: Why run through the bespoke memoized rather than just assigning immediately? I can't think of it as an optimization (especially because during assignment time the which binary and relevant paths are bound to be fresh in the OS cache), and don't see any other effect.

Just that since this is actually rarely used using memoized saves up on a make $(shell) call, it's one of those little impact optimizations, but if they were used everywhere could speed up a bit some things, kind of a good practice.

@fjmolinas
Copy link
Contributor Author

Another question to understand the patch: Why run through the bespoke memoized rather than just assigning immediately? I can't think of it as an optimization (especially because during assignment time the which binary and relevant paths are bound to be fresh in the OS cache), and don't see any other effect.

Just that since this is actually rarely used using memoized saves up on a make $(shell) call, it's one of those little impact optimizations, but if they were used everywhere could speed up a bit some things, kind of a good practice.

Its a bit explained here https://doc.riot-os.org/build-system-basics.html#variable-declaration-guidelines, but its basically that all $(shell) have a bit of overhead, when using := this happens only once, so its more impactful when it's = $(shell something) since its called every time the variable is deference.

@fjmolinas fjmolinas added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 2, 2021
chrysn
chrysn approved these changes Sep 2, 2021
View reviewed changes
Copy link
Member

@chrysn chrysn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the pointer, and sorry for the delay. Builds pass, and not erring out needlessly on slim systems is a good thing.

@miri64 miri64 merged commit ab6f24f into RIOT-OS:master Sep 2, 2021
@fjmolinas fjmolinas deleted the pr_curl_wget_unzip_7z_no_error branch September 3, 2021 09:23
@benpicco benpicco added this to the Release 2021.10 milestone Oct 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrysn chrysn chrysn approved these changes

@miri64 miri64 Awaiting requested review from miri64

Assignees
No one assigned
Labels
Area: build system Area: Build system CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Milestone
Release 2021.10
Development

Successfully merging this pull request may close these issues.
4 participants
@fjmolinas @chrysn @miri64 @benpicco