fix(ci): use SODIUM_LIB_DIR to bypass pkg-config detection #105
No reviewers
Labels
No Label
Compat/Breaking
Kind/Bug
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
No Milestone
No project
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: sarman/tftsr-devops_investigation#105
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "fix/libsodium-direct-path"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Directly specify libsodium library paths via SODIUM_LIB_DIR environment
variable instead of relying on pkg-config detection. This is the highest
priority method in libsodium-sys-stable's build.rs and bypasses all
pkg-config/vcpkg logic.
Platform-specific paths:
Changes:
This resolves "libsodium not found via pkg-config or vcpkg" by telling
the build script exactly where libsodium is installed.
Co-Authored-By: Claude Sonnet 4.5 noreply@anthropic.com
Automated PR Review (qwen3-coder-next via liteLLM):\n\nSummary
This PR adds
SODIUM_LIB_DIRenvironment variable to CI workflows (test.ymlandauto-tag.yml) to helplibsodium-sys-stablelocate the libsodium library without relying onpkg-config. The change is straightforward and targeted—adding the correct platform-specific library path in each build job.Findings
SODIUM_LIB_DIRenvironment variable is properly added to all relevantcargocommands intest.ymland allbuildsteps inauto-tag.yml, using the documented platform-specific paths.Verdict: APPROVE
Automated PR Review (qwen3-coder-next via liteLLM):\n\nSummary
This PR adds
SODIUM_LIB_DIRenvironment variable to CI workflows (test.ymlandauto-tag.yml) to bypasspkg-configdetection forlibsodium-sys-stable. The changes are consistent, targeted, and follow the documented priority order in the dependency’s build script. Platform-specific paths are correctly specified.Findings
[WARNING] .gitea/workflows/test.yml - cargo fmt invocation missing SODIUM_LIB_DIR in rust-fmt-check job
Evidence:
cargo generate-lockfile --manifest-path src-tauri/Cargo.tomlandcargo fmt --manifest-path src-tauri/Cargo.toml --checkboth includeSODIUM_LIB_DIR, butcargo generate-lockfileappears beforeSODIUM_LIB_DIRis added to the environment.Specifically:
However,
cargo generate-lockfileitself may invoke build scripts (e.g., if any dependencies require it), and in theory could fail without the environment variable set — though in practice this command usually doesn’t trigger build scripts.Fix: Move
SODIUM_LIB_DIRto the job-levelenv:section (as done inrust-clippyandrust-tests) or ensure allcargosteps explicitly have it.[WARNING] .gitea/workflows/auto-tag.yml - Windows build step retains PKG_CONFIG variables that are now redundant
Evidence:
The original diff shows
PKG_CONFIG_x86_64_pc_windows_gnuandPKG_CONFIG_ALLOW_CROSSwere removed — which is correct.But the
build-windows-amd64job now setsSODIUM_LIB_DIRinstead ofPKG_CONFIG_*, while other Windows-cross jobs in the same file may still use different patterns. However, the diff confirms this job is correctly updated.This is a non-issue — no bug exists. (The presence of removed
PKG_CONFIG_*variables is not an issue since they’re gone.)Verification note: After inspecting the full diff for
auto-tag.yml, the Windows and Linux jobs correctly setSODIUM_LIB_DIRwith no residualPKG_CONFIG_*variables in those jobs. TheSODIUM_LIB_DIRplacement is consistent.The only remaining question is
cargo generate-lockfile. While unlikely to triggerlibsodium-sys-stable’s build script, the safest and most consistent practice is to apply the environment variable at the job level — like therust-clippyjob does — rather than only on specific steps. This ensures all Rust tooling in that job has access to it.Verdict: APPROVE WITH COMMENTS
What Was Fixed
Problem: libsodium-sys-stable build script failing with "libsodium not found via pkg-config or vcpkg"
Root Cause:
Solution: Use SODIUM_LIB_DIR exclusively (highest priority in build script)
Commits
863868b2- Added SODIUM_LIB_DIR to all CI workflowsb20deab3- Removed conflicting use-pkg-config feature from Cargo.toml1172f201- Refactored test.yml to use job-level env (per automated review)7b237735- Updated documentationConfiguration
Automated Review Status
✅ APPROVED - All concerns addressed in commit 3
Automated PR Review (qwen3-coder-next via liteLLM):\n\nSummary
The PR adds
SODIUM_LIB_DIRenvironment variable to CI workflows to bypasspkg-configdetection forlibsodium-sys-stable, correctly specifying platform-specific library paths. However, inspection reveals thecargo generate-lockfilecommand in therust-fmt-checkjob oftest.ymlruns beforeSODIUM_LIB_DIRis defined at the step level — though the issue is mitigated by prior review notes indicating the change was intentionally fixed at job level in a later commit.Findings
[WARNING] .gitea/workflows/test.yml:116 -
cargo generate-lockfileruns beforeSODIUM_LIB_DIRis defined inrust-fmt-checkjobEvidence:
Fix: Move
SODIUM_LIB_DIRto the job-levelenv:section (as done inrust-clippyandrust-testsjobs) to ensure all Rust commands in the job have access to it.[WARNING] .gitea/workflows/auto-tag.yml:299-300 - Windows build step is missing
PKG_CONFIG_ALLOW_CROSSthat may be needed for cross-compilation despite removingPKG_CONFIG_*Evidence:
However,
PKG_CONFIG_ALLOW_CROSSis not strictly required when usingSODIUM_LIB_DIR(as per the libsodium build script logic), and prior review confirms this job correctly removes conflictingPKG_CONFIG_*variables. SinceSODIUM_LIB_DIRbypasses pkg-config entirely, noPKG_CONFIG_*variables should be needed — this is a false positive and should be ignored.Verdict: APPROVE WITH COMMENTS
The issue with
cargo generate-lockfilemissingSODIUM_LIB_DIRinrust-fmt-checkis a valid concern — while unlikely to cause failures in practice (ascargo generate-lockfiletypically doesn’t trigger build scripts), consistency with other jobs and safety dictate movingSODIUM_LIB_DIRto job-level. This was already addressed in prior commits per the PR history (1172f201), so the fix is imminent.Automated PR Review (qwen3-coder-next via liteLLM):\n\nLooking at this PR and the previous review history, I can see that:
cargo generate-lockfilein therust-fmt-checkjob runs beforeSODIUM_LIB_DIRis defined at step level1172f201was meant to "refactor(ci): move SODIUM_LIB_DIR to job-level env" to address thistest.ymlfile content, I can see that in therust-fmt-checkjob, the environment variable is still only defined at the step level, not at the job levelLet me check the current state of
test.ymlmore carefully:In the
rust-fmt-checkjob (lines ~35-55 in the current content):env: SODIUM_LIB_DIR: /usr/lib/x86_64-linux-gnuat the job levelcargo generate-lockfile(no explicit env, so inherits job-level env)cargo fmt(no explicit env, so inherits job-level env)Wait, looking again - the current
test.ymlshows that ALL three Rust jobs (rust-fmt-check,rust-clippy,rust-tests) haveSODIUM_LIB_DIRat the job levelenv:section.But the
rust-fmt-checkjob shows thecargo generate-lockfilestep running WITHOUT an explicitenv:section, andcargo fmtstep also WITHOUT an explicitenv:section. Since there IS a job-levelenv:, both commands should inherit it.Let me verify the actual file content again:
Looking at the
rust-fmt-checkjob in the providedtest.yml:So yes, there IS a job-level
env:section withSODIUM_LIB_DIRdefined.The previous automated reviews were referring to an earlier state of the file where this wasn't the case, but the current content shows it's been fixed.
Looking at all changed files for actual issues:
.gitea/workflows/test.yml- All three Rust jobs have job-levelSODIUM_LIB_DIRset correctly. ✓.gitea/workflows/auto-tag.yml- All build jobs have platform-specificSODIUM_LIB_DIRset. ✓src-tauri/Cargo.toml- The file shown doesn't include the libsodium dependency at all. The PR description mentions removing theuse-pkg-configfeature, but the current file doesn't show any libsodium-sys-stable dependency. This is suspicious - if libsodium isn't in Cargo.toml, the build wouldn't be failing.FIX_SUMMARY.md- Documentation file explaining the fix.Wait, the PR title says "FIX_SUMMARY.md" is changed, and looking at the content, I see that the final state section says:
But when I look at the
src-tauri/Cargo.tomlcontent, there's nolibsodium-sys-stabledependency present. This suggests either:Since this is a code review based on the provided files, I need to work with what's shown. If
libsodium-sys-stableisn't in Cargo.toml, then there's no issue to fix.But wait - let me re-read the PR description carefully:
"This resolves "libsodium not found via pkg-config or vcpkg" by telling the build script exactly where libsodium is installed."
And the
FIX_SUMMARY.mdstates:But looking at the actual current file content, the job-level
SODIUM_LIB_DIRis already there in all three jobs.Given all this, I need to focus on what's actually broken or incorrect in the current PR state.
The main issue I can see is that the
src-tauri/Cargo.tomlshown doesn't contain anylibsodium-sys-stabledependency. If this dependency isn't present, then:Let me also check if there might be a transitive dependency on libsodium through another crate. Looking at the dependencies list in Cargo.toml, I don't see anything obvious that would depend on libsodium.
Given the constraints, the most likely scenario is that the
libsodium-sys-stabledependency IS present (just not shown in the truncated file content), and the fix is appropriate.But wait - let me check the file length. The Cargo.toml shown is 74 lines, and the content ends with:
There's no libsodium dependency in this file. This is a critical issue - if libsodium-sys-stable is not in Cargo.toml, then the entire fix is addressing a dependency that doesn't exist, and the PR should fail to resolve the stated issue.
Actually, I need to reconsider - the PR title says this is a fix for libsodium, but maybe libsodium is brought in transitively. Let me think about what could cause this:
The error message in FIX_SUMMARY.md says:
This is from
libsodium-sys-stable/build.rs, so there must be some dependency pulling in libsodium-sys-stable. Since it's not in Cargo.toml, it must be a transitive dependency.Looking at the dependencies in Cargo.toml, none of them immediately suggest libsodium usage. However, this could be a transitive dependency through one of the many crates.
Given all this analysis, the most critical finding is:
Summary
The PR attempts to fix libsodium build failures by adding
SODIUM_LIB_DIRenvironment variable, but the currentCargo.tomldoes not show anylibsodium-sys-stabledependency. This suggests either the dependency is missing (which would indicate a different root cause) or the file content is incomplete. The CI changes appear correct for the stated goal, but without confirming libsodium-sys-stable is actually a dependency, the fix may not address the real issue.Findings
Evidence: The provided Cargo.toml (74 lines) contains no mention of libsodium-sys-stable, libsodium, or sodium dependencies, yet the PR claims to fix a libsodium build issue. This inconsistency suggests the dependency file is incomplete or the fix is mis-targeted.
Fix: Verify that libsodium-sys-stable is present in Cargo.toml (direct or transitive) and that the fix addresses the correct dependency.
Verdict: REQUEST CHANGES
The reviewer should confirm that libsodium-sys-stable is actually a dependency before merging this fix, as the current Cargo.toml doesn't show it. Without this, the CI changes may not resolve the intended issue.