-
Notifications
You must be signed in to change notification settings - Fork 0
Atavism/linux arm - YAML test #1
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
321e0b6
2f9724d
4786b7e
48f5c46
b938786
0eea169
6f66748
1a3c27a
eca90f6
fa94631
0aa7070
b472338
382f5cc
3ce830f
ba55608
ffba53a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -39,13 +39,20 @@ LINUX_LIB := $(LANTERN_LIB_NAME).so | |||||||||||||||||||||
| LINUX_LIB_AMD64 := $(BIN_DIR)/linux-amd64/$(LANTERN_LIB_NAME).so | ||||||||||||||||||||||
| LINUX_LIB_ARM64 := $(BIN_DIR)/linux-arm64/$(LANTERN_LIB_NAME).so | ||||||||||||||||||||||
| LINUX_LIB_BUILD := $(BIN_DIR)/linux/$(LINUX_LIB) | ||||||||||||||||||||||
| LINUX_INSTALLER_DEB := $(INSTALLER_NAME)$(if $(filter-out production,$(BUILD_TYPE)),-$(BUILD_TYPE)).deb | ||||||||||||||||||||||
| LINUX_INSTALLER_RPM := $(INSTALLER_NAME)$(if $(filter-out production,$(BUILD_TYPE)),-$(BUILD_TYPE)).rpm | ||||||||||||||||||||||
| LINUX_INSTALLER_ARCH := $(INSTALLER_NAME)$(if $(filter-out production,$(BUILD_TYPE)),-$(BUILD_TYPE)).pkg.tar.zst | ||||||||||||||||||||||
| LINUX_TARGET_ARCH ?= amd64 | ||||||||||||||||||||||
| LINUX_PACKAGE_ARCH_SUFFIX := $(if $(filter amd64,$(LINUX_TARGET_ARCH)),,-$(LINUX_TARGET_ARCH)) | ||||||||||||||||||||||
| LINUX_INSTALLER_DEB := $(INSTALLER_NAME)$(if $(filter-out production,$(BUILD_TYPE)),-$(BUILD_TYPE))$(LINUX_PACKAGE_ARCH_SUFFIX).deb | ||||||||||||||||||||||
| LINUX_INSTALLER_RPM := $(INSTALLER_NAME)$(if $(filter-out production,$(BUILD_TYPE)),-$(BUILD_TYPE))$(LINUX_PACKAGE_ARCH_SUFFIX).rpm | ||||||||||||||||||||||
| LINUX_INSTALLER_ARCH := $(INSTALLER_NAME)$(if $(filter-out production,$(BUILD_TYPE)),-$(BUILD_TYPE))$(LINUX_PACKAGE_ARCH_SUFFIX).pkg.tar.zst | ||||||||||||||||||||||
| LINUX_SERVICE_NAME := lanternd | ||||||||||||||||||||||
| LINUX_SERVICE_SRC := $(RADIANCE_REPO)/cmd/lanternd | ||||||||||||||||||||||
| LINUX_SERVICE_BUILD_AMD64 := $(BIN_DIR)/linux-amd64/$(LINUX_SERVICE_NAME) | ||||||||||||||||||||||
| LINUX_SERVICE_BUILD_ARM64 := $(BIN_DIR)/linux-arm64/$(LINUX_SERVICE_NAME) | ||||||||||||||||||||||
| LINUX_SERVICE_BUILD_TARGET := $(BIN_DIR)/linux-$(LINUX_TARGET_ARCH)/$(LINUX_SERVICE_NAME) | ||||||||||||||||||||||
| LINUX_BUNDLE_DIR_X64 := build/linux/x64/release/bundle | ||||||||||||||||||||||
| LINUX_BUNDLE_DIR_ARM64 := build/linux/arm64/release/bundle | ||||||||||||||||||||||
| LINUX_CC_AMD64 ?= x86_64-linux-gnu-gcc | ||||||||||||||||||||||
| LINUX_CC_ARM64 ?= aarch64-linux-gnu-gcc | ||||||||||||||||||||||
| LINUX_PKG_ROOT := linux/packaging | ||||||||||||||||||||||
| LINUX_SERVICE_DST := $(LINUX_PKG_ROOT)/usr/sbin | ||||||||||||||||||||||
| LINUX_PKG_SYSTEMD_DIR := $(LINUX_PKG_ROOT)/usr/lib/systemd/system | ||||||||||||||||||||||
|
|
@@ -260,20 +267,20 @@ install-linux-deps: | |||||||||||||||||||||
| linux-arm64: $(LINUX_LIB_ARM64) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| $(LINUX_LIB_ARM64): $(GO_SOURCES) | ||||||||||||||||||||||
| CC=aarch64-linux-gnu-gcc GOARCH=arm64 LIB_NAME=$@ make desktop-lib | ||||||||||||||||||||||
| CC=$(LINUX_CC_ARM64) GOOS=linux GOARCH=arm64 LIB_NAME=$@ $(MAKE) desktop-lib | ||||||||||||||||||||||
|
Comment on lines
269
to
+270
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/usr/bin/env bash
set -euo pipefail
echo "Linux lib target recipes:"
nl -ba Makefile | sed -n '266,281p'
echo
echo "desktop-lib recipe:"
nl -ba Makefile | sed -n '175,181p'
echo
echo "clean recipe:"
nl -ba Makefile | sed -n '618,624p'Repository: coderabbit-test/lantern Length of output: 152 🏁 Script executed: #!/usr/bin/env bash
set -euo pipefail
echo "=== Linux lib target recipes (lines 266-281) ==="
sed -n '266,281p' Makefile | cat -n
echo
echo "=== desktop-lib recipe (lines 175-181) ==="
sed -n '175,181p' Makefile | cat -n
echo
echo "=== clean recipe (lines 618-624) ==="
sed -n '618,624p' Makefile | cat -n
echo
echo "=== Checking for MKDIR_P definition ==="
grep -n "MKDIR_P" Makefile | head -20Repository: coderabbit-test/lantern Length of output: 2123 🏁 Script executed: #!/bin/bash
echo "=== Searching for LINUX_LIB_ARM64 and LINUX_LIB_AMD64 definitions ==="
grep -n "LINUX_LIB_ARM64\|LINUX_LIB_AMD64" Makefile | grep -v "^\s*#" | head -30
echo
echo "=== Searching for BIN_DIR definition ==="
grep -n "^BIN_DIR" Makefile
echo
echo "=== Searching for LINUX_LIB definition ==="
grep -n "^LINUX_LIB" Makefile | head -10Repository: coderabbit-test/lantern Length of output: 750 Create the per-arch lib output directories before calling These targets write to nested paths like 🛠️ Proposed fix $(LINUX_LIB_ARM64): $(GO_SOURCES)
+ $(call MKDIR_P,$(dir $@))
CC=$(LINUX_CC_ARM64) GOOS=linux GOARCH=arm64 LIB_NAME=$@ $(MAKE) desktop-lib
$(LINUX_LIB_AMD64): $(GO_SOURCES)
+ $(call MKDIR_P,$(dir $@))
CC=$(LINUX_CC_AMD64) GOOS=linux GOARCH=amd64 LIB_NAME=$@ $(MAKE) desktop-lib🤖 Prompt for AI Agents |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| .PHONY: linux-amd64 | ||||||||||||||||||||||
| linux-amd64: $(LINUX_LIB_AMD64) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| $(LINUX_LIB_AMD64): $(GO_SOURCES) | ||||||||||||||||||||||
| CC=x86_64-linux-gnu-gcc GOARCH=amd64 LIB_NAME=$@ make desktop-lib | ||||||||||||||||||||||
| CC=$(LINUX_CC_AMD64) GOOS=linux GOARCH=amd64 LIB_NAME=$@ $(MAKE) desktop-lib | ||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: The amd64 library build has the same missing-directory issue: it writes to Severity Level: Critical 🚨- ❌ Linux amd64 release build can fail immediately.
- ❌ Primary CI release workflow becomes unstable.
Suggested change
Steps of Reproduction ✅1. Trigger Linux CI build at `.github/workflows/build-linux.yml:155` with
`matrix.arch=amd64` (`:27`, passed to `LINUX_TARGET_ARCH` at `:160`).
2. `linux-release-ci` (`Makefile:318`) depends on `linux` (`:279`), which resolves to
`linux-amd64` and runs rule `$(LINUX_LIB_AMD64):` at `:275-276`.
3. Rule line `Makefile:276` invokes `desktop-lib` directly; `desktop-lib` writes output
via `go build ... -o $(LIB_NAME)` (`Makefile:177`) and does not create parent directories.
4. In clean CI workspaces, `bin/linux-amd64/` is absent, so writing
`bin/linux-amd64/liblantern.so` can fail with "no such file or directory".Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** Makefile
**Line:** 276:276
**Comment:**
*Possible Bug: The amd64 library build has the same missing-directory issue: it writes to `bin/linux-amd64/...` without ensuring the directory exists first. This breaks clean CI/local runs where that path has not been created yet.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| .PHONY: linux | ||||||||||||||||||||||
| linux: linux-amd64 | ||||||||||||||||||||||
| linux: linux-$(LINUX_TARGET_ARCH) | ||||||||||||||||||||||
| mkdir -p $(BIN_DIR)/linux | ||||||||||||||||||||||
| cp $(LINUX_LIB_AMD64) $(LINUX_LIB_BUILD) | ||||||||||||||||||||||
| cp $(BIN_DIR)/linux-$(LINUX_TARGET_ARCH)/$(LINUX_LIB) $(LINUX_LIB_BUILD) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| .PHONY: linux-service-amd64 linux-service-arm64 stage-linux-service | ||||||||||||||||||||||
| .PHONY: linux-service-amd64 linux-service-arm64 linux-service stage-linux-service | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| linux-service-amd64: $(GO_SOURCES) | ||||||||||||||||||||||
| $(call MKDIR_P,$(dir $(LINUX_SERVICE_BUILD_AMD64))) | ||||||||||||||||||||||
|
|
@@ -291,10 +298,12 @@ linux-service-arm64: $(GO_SOURCES) | |||||||||||||||||||||
| -o $(LINUX_SERVICE_BUILD_ARM64) $(LINUX_SERVICE_SRC) | ||||||||||||||||||||||
| @echo "Built Linux service: $(LINUX_SERVICE_BUILD_ARM64)" | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| stage-linux-service: linux-service-amd64 | ||||||||||||||||||||||
| linux-service: linux-service-$(LINUX_TARGET_ARCH) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| stage-linux-service: linux-service | ||||||||||||||||||||||
| @echo "Staging systemd unit + service binary $(LINUX_PKG_ROOT)..." | ||||||||||||||||||||||
| $(call MKDIR_P,$(LINUX_SERVICE_DST)) | ||||||||||||||||||||||
| $(call COPY_FILE,$(LINUX_SERVICE_BUILD_AMD64),$(LINUX_SERVICE_DST)/$(LINUX_SERVICE_NAME)) | ||||||||||||||||||||||
| $(call COPY_FILE,$(LINUX_SERVICE_BUILD_TARGET),$(LINUX_SERVICE_DST)/$(LINUX_SERVICE_NAME)) | ||||||||||||||||||||||
| $(call MKDIR_P,$(LINUX_PKG_SYSTEMD_DIR)) | ||||||||||||||||||||||
| $(call COPY_FILE,$(LINUX_SYSTEMD_UNIT_SRC),$(LINUX_SYSTEMD_UNIT_DST)) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
@@ -309,21 +318,29 @@ linux-release: clean linux-release-ci | |||||||||||||||||||||
| linux-release-ci: linux pubget gen | ||||||||||||||||||||||
| @echo "Building Flutter app (release) for Linux..." | ||||||||||||||||||||||
| flutter build linux --release $(DART_DEFINES) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| cp $(LINUX_LIB_BUILD) build/linux/x64/release/bundle | ||||||||||||||||||||||
| $(MAKE) stage-linux-service | ||||||||||||||||||||||
| patchelf --set-rpath '$$ORIGIN/lib' build/linux/x64/release/bundle/lantern || true | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @echo "Packaging deb, rpm, and archlinux with nfpm..." | ||||||||||||||||||||||
| VERSION=$(APP_VERSION) SYSTEMD_UNIT_SRC=$(LINUX_SYSTEMD_UNIT_DST) \ | ||||||||||||||||||||||
| @if [ "$(LINUX_TARGET_ARCH)" = "arm64" ]; then \ | ||||||||||||||||||||||
| BUNDLE_DIR="$(LINUX_BUNDLE_DIR_ARM64)"; \ | ||||||||||||||||||||||
| else \ | ||||||||||||||||||||||
| BUNDLE_DIR="$(LINUX_BUNDLE_DIR_X64)"; \ | ||||||||||||||||||||||
| fi; \ | ||||||||||||||||||||||
| if [ ! -d "$$BUNDLE_DIR" ]; then \ | ||||||||||||||||||||||
| echo "Expected Linux bundle dir not found: $$BUNDLE_DIR"; \ | ||||||||||||||||||||||
| exit 1; \ | ||||||||||||||||||||||
| fi; \ | ||||||||||||||||||||||
| echo "Using Linux bundle dir: $$BUNDLE_DIR"; \ | ||||||||||||||||||||||
| cp "$(LINUX_LIB_BUILD)" "$$BUNDLE_DIR"; \ | ||||||||||||||||||||||
| patchelf --set-rpath '$$ORIGIN/lib' "$$BUNDLE_DIR/lantern" || true; \ | ||||||||||||||||||||||
| VERSION=$(APP_VERSION) GOARCH=$(LINUX_TARGET_ARCH) LINUX_BUNDLE_SRC="$$BUNDLE_DIR/" SYSTEMD_UNIT_SRC=$(LINUX_SYSTEMD_UNIT_DST) \ | ||||||||||||||||||||||
| LANTERND_SRC=$(LINUX_SERVICE_DST)/$(LINUX_SERVICE_NAME) LANTERND_DST=/usr/sbin/$(LINUX_SERVICE_NAME) \ | ||||||||||||||||||||||
| nfpm package -f $(LINUX_PKG_ROOT)/nfpm.yaml -p deb -t $(LINUX_INSTALLER_DEB) | ||||||||||||||||||||||
| VERSION=$(APP_VERSION) SYSTEMD_UNIT_SRC=$(LINUX_SYSTEMD_UNIT_DST) \ | ||||||||||||||||||||||
| nfpm package -f $(LINUX_PKG_ROOT)/nfpm.yaml -p deb -t $(LINUX_INSTALLER_DEB); \ | ||||||||||||||||||||||
| VERSION=$(APP_VERSION) GOARCH=$(LINUX_TARGET_ARCH) LINUX_BUNDLE_SRC="$$BUNDLE_DIR/" SYSTEMD_UNIT_SRC=$(LINUX_SYSTEMD_UNIT_DST) \ | ||||||||||||||||||||||
| LANTERND_SRC=$(LINUX_SERVICE_DST)/$(LINUX_SERVICE_NAME) LANTERND_DST=/usr/sbin/$(LINUX_SERVICE_NAME) \ | ||||||||||||||||||||||
| nfpm package -f $(LINUX_PKG_ROOT)/nfpm.yaml -p rpm -t $(LINUX_INSTALLER_RPM) | ||||||||||||||||||||||
| VERSION=$(APP_VERSION) SYSTEMD_UNIT_SRC=$(LINUX_SYSTEMD_UNIT_DST) \ | ||||||||||||||||||||||
| nfpm package -f $(LINUX_PKG_ROOT)/nfpm.yaml -p rpm -t $(LINUX_INSTALLER_RPM); \ | ||||||||||||||||||||||
|
Comment on lines
+337
to
+340
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: The three packaging commands are chained with Severity Level: Major
|
||||||||||||||||||||||
| nfpm package -f $(LINUX_PKG_ROOT)/nfpm.yaml -p deb -t $(LINUX_INSTALLER_DEB); \ | |
| VERSION=$(APP_VERSION) GOARCH=$(LINUX_TARGET_ARCH) LINUX_BUNDLE_SRC="$$BUNDLE_DIR/" SYSTEMD_UNIT_SRC=$(LINUX_SYSTEMD_UNIT_DST) \ | |
| LANTERND_SRC=$(LINUX_SERVICE_DST)/$(LINUX_SERVICE_NAME) LANTERND_DST=/usr/sbin/$(LINUX_SERVICE_NAME) \ | |
| nfpm package -f $(LINUX_PKG_ROOT)/nfpm.yaml -p rpm -t $(LINUX_INSTALLER_RPM) | |
| VERSION=$(APP_VERSION) SYSTEMD_UNIT_SRC=$(LINUX_SYSTEMD_UNIT_DST) \ | |
| nfpm package -f $(LINUX_PKG_ROOT)/nfpm.yaml -p rpm -t $(LINUX_INSTALLER_RPM); \ | |
| nfpm package -f $(LINUX_PKG_ROOT)/nfpm.yaml -p deb -t $(LINUX_INSTALLER_DEB) && \ | |
| VERSION=$(APP_VERSION) GOARCH=$(LINUX_TARGET_ARCH) LINUX_BUNDLE_SRC="$$BUNDLE_DIR/" SYSTEMD_UNIT_SRC=$(LINUX_SYSTEMD_UNIT_DST) \ | |
| LANTERND_SRC=$(LINUX_SERVICE_DST)/$(LINUX_SERVICE_NAME) LANTERND_DST=/usr/sbin/$(LINUX_SERVICE_NAME) \ | |
| nfpm package -f $(LINUX_PKG_ROOT)/nfpm.yaml -p rpm -t $(LINUX_INSTALLER_RPM) && \ |
Steps of Reproduction ✅
1. CI executes `make linux-release-ci` at `.github/workflows/build-linux.yml:155`, running
packaging recipe at `Makefile:335-343`.
2. The recipe chains three `nfpm package` commands with `;` (`Makefile:337`, `:340`,
`:343`) in one shell line.
3. `Makefile` does not set global shell fail-fast (`.ONESHELL`/`SHELLFLAGS -e` not
defined; only unrelated local `@set -e` appears in iOS block at `Makefile:555`), so the
shell returns the status of the last command.
4. If `deb` or `rpm` packaging fails but `archlinux` succeeds, this line exits success; CI
can proceed and artifact upload for rpm (`.github/workflows/build-linux.yml:256-257`) may
silently publish incomplete outputs.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** Makefile
**Line:** 337:340
**Comment:**
*Logic Error: The three packaging commands are chained with `;`, so failure in the deb/rpm packaging steps can be silently ignored if the final archlinux command succeeds. This can produce partially broken release artifacts while the target still exits successfully.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The three nfpm commands are very similar and repeat a long list of environment variables. This repetition can make the recipe harder to read and maintain. You can define a shell variable to hold the common environment variables to reduce duplication and improve clarity.
NFPM_ENV="VERSION=$(APP_VERSION) GOARCH=$(LINUX_TARGET_ARCH) LINUX_BUNDLE_SRC='$$BUNDLE_DIR/' SYSTEMD_UNIT_SRC=$(LINUX_SYSTEMD_UNIT_DST) LANTERND_SRC=$(LINUX_SERVICE_DST)/$(LINUX_SERVICE_NAME)"; \
$$NFPM_ENV LANTERND_DST=/usr/sbin/$(LINUX_SERVICE_NAME) \
nfpm package -f $(LINUX_PKG_ROOT)/nfpm.yaml -p deb -t $(LINUX_INSTALLER_DEB); \
$$NFPM_ENV LANTERND_DST=/usr/sbin/$(LINUX_SERVICE_NAME) \
nfpm package -f $(LINUX_PKG_ROOT)/nfpm.yaml -p rpm -t $(LINUX_INSTALLER_RPM); \
$$NFPM_ENV LANTERND_DST=/usr/bin/$(LINUX_SERVICE_NAME) \
nfpm package -f $(LINUX_PKG_ROOT)/nfpm.yaml -p archlinux -t $(LINUX_INSTALLER_ARCH)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: The arm64 library build writes output to
bin/linux-arm64/...but never creates that directory first. On clean builds this will fail with a "no such file or directory" error whengo build -otries to write the shared library. [resource leak]Severity Level: Critical 🚨
Steps of Reproduction ✅
Prompt for AI Agent 🤖