CI: Reduce HTTP requests when possible #560
Conversation
Code Review Agent Run #985d6cActionable Suggestions - 1
Review Details
AI Code Review powered by |
Changelist by BitoThis pull request implements the following key changes.
|
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
jserv
force-pushed
the
ci-skip-checksum
branch
from
3aad2be to
9ffb959
Compare
Code Review Agent Run #47af7aActionable Suggestions - 0Review Details
AI Code Review powered by |
jserv
force-pushed
the
ci-skip-checksum
branch
from
9ffb959 to
878ce74
Compare
Code Review Agent Run #93516dActionable Suggestions - 1
Additional Suggestions - 1
Review Details
AI Code Review powered by |
|
@vacantron, I tried to bypass checksum validation, but it did not work as intended. Can you help me fix this? |
jserv
force-pushed
the
ci-skip-checksum
branch
from
878ce74 to
ea47f27
Compare
Code Review Agent Run #b86493Actionable Suggestions - 0Review Details
AI Code Review powered by |
jserv
force-pushed
the
ci-skip-checksum
branch
2 times, most recently
from
5d017a0 to
e260367
Compare
Code Review Agent Run #5e956aActionable Suggestions - 2
Review Details
AI Code Review powered by |
|
I defer to @vacantron to determine if we should proceed with the proposed changes. |
vacantron
force-pushed
the
ci-skip-checksum
branch
from
e260367 to
0ec87f3
Compare
jserv
changed the title
Code Review Agent Run #b1b752Actionable Suggestions - 2
Additional Suggestions - 2
Review Details
AI Code Review powered by |
|
Append |
|
Mention Arm64 regressions in git commit messages to enable earlier pull request merging despite ongoing CI pipeline issues. |
mk/artifact.mk
Outdated
| wget -q -O $(BIN_DIR)/sha1sum-linux-x86-softfp $(PREBUILT_BLOB_URL)/sha1sum-linux-x86-softfp | ||
| wget -q -O $(BIN_DIR)/sha1sum-riscv32 $(PREBUILT_BLOB_URL)/sha1sum-riscv32 |
There was a problem hiding this comment.
Later, we may consider to introduce function calls to wrap wget in another pull request.
There was a problem hiding this comment.
Sure. Moreover, consolidating the file name of sha1sum and prebuilts, such as rv32emu-prebuilt-*.sha would make it easier.
| run: | | ||
| make artifact | ||
| make ENABLE_SYSTEM=1 artifact | ||
| make ENABLE_ARCH_TEST=1 artifact |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
vacantron
force-pushed
the
ci-skip-checksum
branch
from
0ec87f3 to
c8c8263
Compare
Code Review Agent Run #6bfd95Actionable Suggestions - 0Additional Suggestions - 2
Review Details
AI Code Review powered by |
Check prebuilt tarballs are existing or not first. If so, skip the downloading of releases tags and tarballs. In the CI, we first call "make artifact" to prepare the tarballs, so the GitHub API for fetching releases tags would not be called and be skipped when "make" is called next time. After this commit, wget in the CI would not complain about error code 403 (rate limit exceeded) anymore. However, the build on Aarch64 still failed because of other reansons, e.g. software bugs of GCC or QEMU. Close #536
vacantron
force-pushed
the
ci-skip-checksum
branch
from
c8c8263 to
c85279e
Compare
Code Review Agent Run #5f64b2Actionable Suggestions - 1
Review Details
AI Code Review powered by |
| 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 |
There was a problem hiding this comment.
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
| 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
|
The new patch to solve the Arm64 CI failure will be opened after this pull request merged. https://github.com/sysprog21/rv32emu/actions/runs/13257429033/job/37006598258?pr=560 |
|
Thank @vacantron for contributing! |
Due to frequent CI failures during checksum verification, this commit bypasses integrity checks by assuming all downloaded files are complete, removing the requirement to download and verify checksums.
Summary by Bito
This PR enhances the CI build process by optimizing HTTP requests and file download handling through the introduction of a fetch-releases-tag function. The changes include improved file existence checks to prevent redundant downloads, streamlined build processes with better checksum verification, and enhanced cleanup procedures in the Makefile.Unit tests added: False
Estimated effort to review (1-5, lower is better): 2