feat: allow multiple architectures from netinstall-cli 7.16#5
feat: allow multiple architectures from netinstall-cli 7.16#5clorichel wants to merge 2 commits intotikoci:masterfrom
Conversation
|
Thanks! Sorry I still have to tested - but looks good. And you even handle... FIRST_MULTIARCH_NETINSTALL_VER ?= 7.16. I recall something about x86 package being different... so I cannot even remember they worked before you changes. |
There was a problem hiding this comment.
Pull request overview
This PR updates the project’s Makefile to support supplying multiple RouterOS architectures (as a list) so netinstall-cli can select the correct package without restarting the container, aligning with netinstall-cli 7.16+ multi-arch behavior.
Changes:
- Allow
ARCHto be a space-separated list and expandPKGS_FILESacross all architectures. - Add a version gate intended to prevent multi-arch usage with
netinstall-cliversions older than 7.16. - Update download targets to fetch
routerosandall_packagesartifacts via pattern rules for multiple architectures.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1,4 +1,4 @@ | |||
| ARCH ?= arm | |||
| ARCH ?= arm arm64 | |||
There was a problem hiding this comment.
With a multi-item default like ARCH ?= arm arm64, any recursive $(MAKE) ... ARCH=$(ARCH) invocations will be parsed by the shell as multiple words (e.g., make ... ARCH=arm arm64 ...), causing arm64 to be treated as an extra make goal and dropping the intended multi-arch value. Quote/escape the assignment in recursive make calls (e.g., ARCH='$(ARCH)') or avoid space-separated lists for ARCH in those paths.
| @@ -1,4 +1,4 @@ | |||
| ARCH ?= arm | |||
| ARCH ?= arm arm64 | |||
There was a problem hiding this comment.
Changing the default from a single ARCH to arm arm64 increases the default downloads/work (multiple routeros/all_packages artifacts) and makes the README’s documented defaults (e.g., ARCH ?= arm) inaccurate. If the new default is intentional, the documentation/config guidance should be updated; otherwise consider keeping a single-arch default and letting users opt into multi-arch explicitly.
| define compare_versions | ||
| $(if $(findstring $(word 1,$(sort $(1) $(2))),$(2)),true,false) | ||
| endef |
There was a problem hiding this comment.
compare_versions returns the literal strings true/false. In GNU make, $(if ...) treats any non-empty string (including false) as truthy, so the version gate at run will never take the error branch. Also, using sort + findstring is not a reliable version comparator (e.g., 7.10 vs 7.9, and substring cases like 7.1 vs 7.16). Consider returning an empty string for the false case and using a real version comparison (e.g., sort -V in a $(shell ...) helper) that can handle rc/beta suffixes correctly.
| $(if $(call compare_versions,$(VER_NETINSTALL),$(FIRST_MULTIARCH_NETINSTALL_VER)), , \ | ||
| $(if $(findstring ,$(wordlist 2,2,$(ARCH))),, \ | ||
| $(error "You cannot have multiple ARCH items if you use netinstall-cli < 7.16"))) |
There was a problem hiding this comment.
This check for a second ARCH item is broken: $(findstring ,$(wordlist 2,2,$(ARCH))) uses an empty search string, which evaluates empty and will not correctly detect multi-item ARCH values. Use a non-empty test like $(word 2,$(ARCH)) (or $(words $(ARCH)) > 1) so the error only triggers when multiple architectures are actually configured.
| $(if $(findstring ,$(wordlist 2,2,$(ARCH))),, \ | ||
| $(error "You cannot have multiple ARCH items if you use netinstall-cli < 7.16"))) | ||
|
|
||
| $(if $(findstring x86_64, $(PLATFORM)), , $(QEMU)) ./netinstall-cli-$(VER_NETINSTALL) $(OPTS) $(NET_OPTS) routeros-$(VER)-$(ARCH).npk $(PKGS_FILES) $(PKGS_CUSTOM) |
There was a problem hiding this comment.
routeros-$(VER)-$(ARCH).npk will break when ARCH contains multiple items (it expands to a space-separated string, which the shell splits into multiple arguments and invalid filenames). If multi-arch netinstall is intended, build a list of routeros NPKs similar to PKGS_FILES and pass that list instead of interpolating the raw ARCH variable into a single filename.
| all: $(foreach somearch,$(ARCH),routeros-$(VER)-$(somearch).npk netinstall-cli-$(VER_NETINSTALL) all_packages-$(somearch)-$(VER).zip) | ||
| @echo finished download ARCH=$(ARCH) VER=$(VER) PKGS=$(PKGS) PLATFORM=$(PLATFORM) |
There was a problem hiding this comment.
all prerequisites are generated with a foreach that includes netinstall-cli-$(VER_NETINSTALL) inside the loop, duplicating it once per architecture. It’s cleaner and less error-prone to list netinstall-cli-$(VER_NETINSTALL) once, plus per-arch routeros/all_packages prerequisites separately.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
1 similar comment
|
@copilot open a new pull request to apply changes based on the comments in this thread |
see #4