[WIP] Install OpenSSL and pkg-config dependencies in Linux CI workflow#93
Open
HsiangNianian wants to merge 11 commits intomainfrom
Open
[WIP] Install OpenSSL and pkg-config dependencies in Linux CI workflow#93HsiangNianian wants to merge 11 commits intomainfrom
HsiangNianian wants to merge 11 commits intomainfrom
Conversation
Co-authored-by: NtskwK <natsukawa247@outlook.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Added installation of pkg-config and libssl-dev for musl target.
Removed installation of Musl tools for musl target and added installation of OpenSSL dev package.
Added installation steps for Musl tools in CI workflow.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds a new multilingual documentation site (English and Chinese) using Fumadocs/React Router, updates top-level project docs for i18n and contribution info, and adjusts the semifold CI workflow to ensure required OpenSSL and musl tooling is installed for Linux builds. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
Workspace change through: d4cf2330 changesets found Planned changes to release
|
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the Semifold CI workflow configuration (likely as part of a branch sync) by adjusting build-matrix entries and Linux package installation steps.
Changes:
- Reorders the musl matrix entry fields (moves
argsbelowinstall-musl). - Adds an OpenSSL dev package install step and tweaks musl tooling installation (adds
pkg-config). - Minor whitespace adjustments in the workflow.
Comment on lines
76
to
+77
| sudo apt-get install -y musl-tools musl-dev | ||
|
|
||
| sudo apt-get install pkg-config |
There was a problem hiding this comment.
sudo apt-get install pkg-config is missing -y, which can cause the workflow to prompt for confirmation and hang in CI. Use the non-interactive flag (and ideally install it in the same apt invocation as the other packages).
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The new 'Install OpenSSL dev package' step will run on all matrix platforms; consider adding an
if: runner.os == 'Linux'guard (or similar) soapt-getisn’t invoked on non-Linux runners. - You now install
libssl-devboth in the dedicated OpenSSL step and again in the 'Install Dependencies (Linux x86_64)' step; consolidating this into one place will simplify the workflow and avoid redundant package installs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new 'Install OpenSSL dev package' step will run on all matrix platforms; consider adding an `if: runner.os == 'Linux'` guard (or similar) so `apt-get` isn’t invoked on non-Linux runners.
- You now install `libssl-dev` both in the dedicated OpenSSL step and again in the 'Install Dependencies (Linux x86_64)' step; consolidating this into one place will simplify the workflow and avoid redundant package installs.
## Individual Comments
### Comment 1
<location> `.github/workflows/semifold-ci.yaml:64-65` </location>
<code_context>
runs-on: ${{ matrix.platform }}
steps:
- uses: actions/checkout@v6
+ - name: Install OpenSSL dev package
+ run: sudo apt-get update && sudo apt-get install -y libssl-dev
- name: Install Dependencies (Linux x86_64)
</code_context>
<issue_to_address>
**issue (bug_risk):** Running apt-get unconditionally in this step will fail on non-Linux runners and duplicates the Linux dependency install logic.
This step runs on all matrix platforms but assumes a Debian-based system with `apt-get`, so it will fail on macOS/Windows (and non-apt Linux images). It also duplicates the `libssl-dev` install already covered in the Linux dependencies step. Please either gate this with `if: runner.os == 'Linux'` and merge it into `Install Dependencies (Linux x86_64)`, or remove it and rely on the existing Linux install command to include `libssl-dev`.
</issue_to_address>
### Comment 2
<location> `.github/workflows/semifold-ci.yaml:73-77` </location>
<code_context>
run: |
sudo apt-get install -y musl-tools musl-dev
-
+ sudo apt-get install pkg-config
+
- name: Install pnpm
</code_context>
<issue_to_address>
**suggestion:** Consider combining pkg-config installation with the previous apt-get invocation and using non-interactive flags.
This adds a second `apt-get install` and drops the `-y` flag, which may trigger interactive prompts depending on the image. Please add `pkg-config` to the existing `apt-get install -y musl-tools musl-dev` line (e.g., `musl-tools musl-dev pkg-config`) so the step stays idempotent and non-interactive.
```suggestion
- name: Install Musl Tools (for musl target)
if: matrix.install-musl == true
run: |
sudo apt-get install -y musl-tools musl-dev pkg-config
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
73
to
+77
| - name: Install Musl Tools (for musl target) | ||
| if: matrix.install-musl == true | ||
| run: | | ||
| sudo apt-get install -y musl-tools musl-dev | ||
|
|
||
| sudo apt-get install pkg-config |
Contributor
There was a problem hiding this comment.
suggestion: Consider combining pkg-config installation with the previous apt-get invocation and using non-interactive flags.
This adds a second apt-get install and drops the -y flag, which may trigger interactive prompts depending on the image. Please add pkg-config to the existing apt-get install -y musl-tools musl-dev line (e.g., musl-tools musl-dev pkg-config) so the step stays idempotent and non-interactive.
Suggested change
| - name: Install Musl Tools (for musl target) | |
| if: matrix.install-musl == true | |
| run: | | |
| sudo apt-get install -y musl-tools musl-dev | |
| sudo apt-get install pkg-config | |
| - name: Install Musl Tools (for musl target) | |
| if: matrix.install-musl == true | |
| run: | | |
| sudo apt-get install -y musl-tools musl-dev pkg-config |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Member
Author
|
@SourceryAI title |
sourcery-ai
bot
changed the title
HsiangNianian
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Update CI workflow to ensure required system dependencies are installed for Linux and musl-based builds.
CI: