Skip to content

CI: Reduce HTTP requests when possible #560

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

Merged
merged 1 commit into from
Feb 11, 2025
Merged

CI: Reduce HTTP requests when possible #560

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion .github/workflows/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,20 @@ jobs:
uses: rlalik/setup-cpp-compiler@master
with:
compiler: ${{ matrix.compiler }}
- name: fetch artifact first to reduce HTTP requests
env:
CC: ${{ steps.install_cc.outputs.cc }}
run: |
make artifact
make ENABLE_SYSTEM=1 artifact
make ENABLE_ARCH_TEST=1 artifact
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since LATEST_RELEASE is guarded to the tarball, I am not sure if fetching all artifacts in a single job is a good choice.

Would not it make more sense to fetch the required artifact in a relevant job instead? If it is not, the line 242 should be refined such that removing the redundant artifact fetching.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is just moving up the artifacts fetching. We do all userspace test, system simulation test, and architecture test below.

The redundant make artifact could not cause any problem. Beccause once the tarball is downloaded, we check that tarball to decide triggering downloading or not.

if: ${{ always() }}
- name: default build with -g
env:
CC: ${{ steps.install_cc.outputs.cc }}
run: make OPT_LEVEL=-g -j$(nproc)
run: |
make distclean
make OPT_LEVEL=-g -j$(nproc)
if: ${{ always() }}
- name: default build with -Og
env:
Expand Down Expand Up @@ -268,6 +278,7 @@ jobs:
./llvm.sh 18
# Append custom commands here
run: |
make artifact
make -j$(nproc)
make check -j$(nproc)
make ENABLE_JIT=1 clean && make ENABLE_JIT=1 check -j$(nproc)
Expand Down
3 changes: 0 additions & 3 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,9 +416,6 @@ endif
clean:
$(RM) $(BIN) $(OBJS) $(DEV_OBJS) $(BUILD_DTB) $(BUILD_DTB2C) $(HIST_BIN) $(HIST_OBJS) $(deps) $(WEB_FILES) $(CACHE_OUT) src/rv32_jit.c
distclean: clean
-$(RM) $(DOOM_DATA) $(QUAKE_DATA) $(BUILDROOT_DATA) $(LINUX_DATA)
$(RM) -r $(OUT)/linux-image
$(RM) -r $(TIMIDITY_DATA)
$(RM) -r $(OUT)/id1
$(RM) -r $(DEMO_DIR)
$(RM) *.zip
Expand Down
66 changes: 49 additions & 17 deletions mk/artifact.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,15 +36,33 @@ SCIMARK2_SHA1 := de278c5b8cef84ab6dda41855052c7bfef919e36

SHELL_HACK := $(shell mkdir -p $(BIN_DIR)/linux-x86-softfp $(BIN_DIR)/riscv32 $(BIN_DIR)/linux-image)

# $(1): tag of GitHub releases
# $(2): name of GitHub releases
# $(3): name showing in terminal
define fetch-releases-tag
$(if $(wildcard $(BIN_DIR)/$(2)), \
$(info $(3) is found. Skipping downloading.), \
$(eval LATEST_RELEASE := $(shell wget -q https://api.github.com/repos/sysprog21/rv32emu-prebuilt/releases -O- \
| grep '"tag_name"' \
| grep "$(1)" \
| head -n 1 \
| sed -E 's/.*"tag_name": "([^"]+)".*/\1/')) \
$(if $(LATEST_RELEASE),, \
$(error Fetching tag of latest releases failed) \
) \
)
endef

ifeq ($(call has, PREBUILT), 1)
ifeq ($(call has, SYSTEM), 1)
LATEST_RELEASE := $(shell wget -q https://api.github.com/repos/sysprog21/rv32emu-prebuilt/releases -O- | grep '"tag_name"' | grep "Linux-Image" | head -n 1 | sed -E 's/.*"tag_name": "([^"]+)".*/\1/')
else ifeq ($(call has, ARCH_TEST), 1)
LATEST_RELEASE := $(shell wget -q https://api.github.com/repos/sysprog21/rv32emu-prebuilt/releases -O- | grep '"tag_name"' | grep "sail" | head -n 1 | sed -E 's/.*"tag_name": "([^"]+)".*/\1/')
else
LATEST_RELEASE := $(shell wget -q https://api.github.com/repos/sysprog21/rv32emu-prebuilt/releases -O- | grep '"tag_name"' | grep "ELF" | head -n 1 | sed -E 's/.*"tag_name": "([^"]+)".*/\1/')
endif
PREBUILT_BLOB_URL = https://github.com/sysprog21/rv32emu-prebuilt/releases/download/$(LATEST_RELEASE)
ifeq ($(call has, SYSTEM), 1)
$(call fetch-releases-tag,Linux-Image,rv32emu-linux-image-prebuilt.tar.gz,Linux image)
else ifeq ($(call has, ARCH_TEST), 1)
$(call fetch-releases-tag,sail,rv32emu-prebuilt-sail-$(HOST_PLATFORM),Sail model)
else
$(call fetch-releases-tag,ELF,rv32emu-prebuilt.tar.gz,Prebuilt benchmark)
endif

PREBUILT_BLOB_URL = https://github.com/sysprog21/rv32emu-prebuilt/releases/download/$(LATEST_RELEASE)
else
# Since rv32emu only supports the dynamic binary translation of integer instruction in tiered compilation currently,
# we disable the hardware floating-point and the related SIMD operation of x86.
Expand Down Expand Up @@ -95,14 +113,15 @@ endif
ifeq ($(call has, ARCH_TEST), 1)
$(Q)if [ "$(RES)" = "1" ]; then \
$(PRINTF) "\n$(YELLOW)SHA-1 verification failed! Re-fetching prebuilt binaries from \"rv32emu-prebuilt\" ...\n$(NO_COLOR)"; \
wget -q --show-progress $(PREBUILT_BLOB_URL)/$(RV32EMU_PREBUILT_TARBALL) -O build/$(RV32EMU_PREBUILT_TARBALL);\
wget -q --show-progress $(PREBUILT_BLOB_URL)/$(RV32EMU_PREBUILT_TARBALL) -O build/$(RV32EMU_PREBUILT_TARBALL); \
else \
$(call notice, [OK]); \
fi
else
$(Q)if [ "$(RES)" = "1" ]; then \
$(PRINTF) "\n$(YELLOW)SHA-1 verification failed! Re-fetching prebuilt binaries from \"rv32emu-prebuilt\" ...\n$(NO_COLOR)"; \
wget -q --show-progress $(PREBUILT_BLOB_URL)/$(RV32EMU_PREBUILT_TARBALL) -O- | tar -C build --strip-components=1 -xz; \
wget -q --show-progress $(PREBUILT_BLOB_URL)/$(RV32EMU_PREBUILT_TARBALL) -O build/$(RV32EMU_PREBUILT_TARBALL); \
tar --strip-components=1 -zxf build/$(RV32EMU_PREBUILT_TARBALL) -C build; \
else \
$(call notice, [OK]); \
fi
Expand Down Expand Up @@ -147,16 +166,29 @@ endif

fetch-checksum:
ifeq ($(call has, PREBUILT), 1)
$(Q)$(PRINTF) "Fetching SHA-1 of prebuilt binaries ...\n"
$(Q)$(PRINTF) "Fetching SHA-1 of prebuilt binaries ... "
ifeq ($(call has, SYSTEM), 1)
$(Q)wget -q -O $(BIN_DIR)/sha1sum-linux-image $(PREBUILT_BLOB_URL)/sha1sum-linux-image
$(Q)$(call notice, [OK])
ifeq ($(wildcard $(BIN_DIR)/rv32emu-linux-image-prebuilt.tar.gz),)
$(Q)wget -q -O $(BIN_DIR)/sha1sum-linux-image $(PREBUILT_BLOB_URL)/sha1sum-linux-image
$(Q)$(call notice, [OK])
else
$(Q)$(PRINTF) "Skipped\n"
endif
else ifeq ($(call has, ARCH_TEST), 1)
$(Q)wget -q -O $(BIN_DIR)/rv32emu-prebuilt-sail-$(HOST_PLATFORM).sha $(PREBUILT_BLOB_URL)/rv32emu-prebuilt-sail-$(HOST_PLATFORM).sha
ifeq ($(wildcard $(BIN_DIR)/rv32emu-prebuilt-sail-$(HOST_PLATFORM)),)
$(Q)wget -q -O $(BIN_DIR)/rv32emu-prebuilt-sail-$(HOST_PLATFORM).sha $(PREBUILT_BLOB_URL)/rv32emu-prebuilt-sail-$(HOST_PLATFORM).sha
$(Q)$(call notice, [OK])
else
$(Q)$(PRINTF) "Skipped\n"
endif
else
$(Q)wget -q -O $(BIN_DIR)/sha1sum-linux-x86-softfp $(PREBUILT_BLOB_URL)/sha1sum-linux-x86-softfp
$(Q)wget -q -O $(BIN_DIR)/sha1sum-riscv32 $(PREBUILT_BLOB_URL)/sha1sum-riscv32
$(Q)$(call notice, [OK])
ifeq ($(wildcard $(BIN_DIR)/rv32emu-prebuilt.tar.gz),)
$(Q)wget -q -O $(BIN_DIR)/sha1sum-linux-x86-softfp $(PREBUILT_BLOB_URL)/sha1sum-linux-x86-softfp
$(Q)wget -q -O $(BIN_DIR)/sha1sum-riscv32 $(PREBUILT_BLOB_URL)/sha1sum-riscv32
$(Q)$(call notice, [OK])
else
$(Q)$(PRINTF) "Skipped\n"
endif
Comment on lines +185 to +191
Copy link

Choose a reason for hiding this comment

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

Consider consolidating repeated conditional blocks

Consider consolidating the conditional block checking for rv32emu-prebuilt.tar.gz with similar patterns found in the code. The same pattern appears multiple times with different file checks which could be refactored into a helper function to improve maintainability.

Code suggestion
Check the AI-generated fix before applying
Suggested change
ifeq ($(wildcard $(BIN_DIR)/rv32emu-prebuilt.tar.gz),)
$(Q)wget -q -O $(BIN_DIR)/sha1sum-linux-x86-softfp $(PREBUILT_BLOB_URL)/sha1sum-linux-x86-softfp
$(Q)wget -q -O $(BIN_DIR)/sha1sum-riscv32 $(PREBUILT_BLOB_URL)/sha1sum-riscv32
$(Q)$(call notice, [OK])
else
$(Q)$(PRINTF) "Skipped\n"
endif
$(call download_if_not_exists,$(BIN_DIR)/rv32emu-prebuilt.tar.gz,$(BIN_DIR)/sha1sum-linux-x86-softfp,$(PREBUILT_BLOB_URL)/sha1sum-linux-x86-softfp)
$(call download_if_not_exists,$(BIN_DIR)/rv32emu-prebuilt.tar.gz,$(BIN_DIR)/sha1sum-riscv32,$(PREBUILT_BLOB_URL)/sha1sum-riscv32)

Code Review Run #5f64b2

Is this a valid issue, or was it incorrectly flagged by the Agent?

  • it was incorrectly flagged

endif
endif

Expand Down
Loading