Skip to content

Skip non-matching platform binaries during resolve#43

Merged
Yangmoooo merged 4 commits intomainfrom
codex/fix-codex-binary-install-linux
Mar 23, 2026
Merged

Skip non-matching platform binaries during resolve#43
Yangmoooo merged 4 commits intomainfrom
codex/fix-codex-binary-install-linux

Conversation

@Codex
Copy link
Copy Markdown
Contributor

@Codex Codex AI commented Mar 23, 2026

Which issue does this PR resolve?

Linux installs for codex failed because the resolver fell back to the package name when platform-specific binaries didn’t match, leading to searches for a non-existent codex binary inside the Linux archive.

Description of this PR

  • Platform filtering: PkgDef::resolve now drops binaries whose platform-specific names don’t match the current platform instead of falling back to the package name, preventing phantom lookups on Linux codex archives.
  • Tests: Updated platform-specific resolution tests to assert skipping unmatched binaries and Linux-only retention of the codex payload.

Example:

let pkg_def = make_pkg_def(vec![BinDef {
    name: Some(PlatformAwareString::ByPlatform(
        [("nonexistent-platform-xyz".into(), "bin".into())].into(),
    )),
    link: None,
}]);

// Previously produced a fallback entry; now yields no binaries on non-matching platforms.
assert!(pkg_def.resolve("codex").bin.is_empty());

@Codex Codex AI changed the title [WIP] Fix codex binary installation issue on Linux platform Skip non-matching platform binaries during resolution Mar 23, 2026
@Codex Codex AI changed the title Skip non-matching platform binaries during resolution Skip non-matching platform binaries during resolve Mar 23, 2026
@Codex Codex AI requested a review from Yangmoooo March 23, 2026 14:03
@Yangmoooo Yangmoooo marked this pull request as ready for review March 23, 2026 15:00
Copilot AI review requested due to automatic review settings March 23, 2026 15:00
@Yangmoooo Yangmoooo merged commit 76ef18f into main Mar 23, 2026
6 of 8 checks passed
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates platform-aware binary resolution so that binaries whose platform-specific names don’t match the current platform are skipped during PkgDef::resolve, preventing incorrect fallback lookups (notably for Linux codex installs).

Changes:

  • Adjust PkgDef::resolve to skip non-matching platform-mapped binary names rather than falling back to the package name.
  • Rename/update a unit test to assert that non-matching platform entries are skipped.
  • Update the codex TOML deserialization + resolution test expectations (Linux keeps only the Linux payload).
Comments suppressed due to low confidence (1)

src/package.rs:129

  • b.name is moved by value in the match, which partially moves b and makes the later access to b.link illegal (this should fail to compile). Borrow b.name (e.g., match &b.name) or destructure BinDef { name, link } up-front so both fields can be used while still allowing resolve_for_platform() returning None to skip the binary.
                    // Resolve name from PlatformAwareString; if it doesn't match the current
                    // platform, skip this binary instead of falling back to the package name.
                    let raw_name = match b.name {
                        Some(s) => s.resolve_for_platform(),
                        None => Some(pkg_name.to_string()),
                    }?;
                    let name = normalize_name(raw_name);

                    // Resolve link from PlatformAwareString, defaulting to the resolved name
                    let raw_link =
                        b.link.as_ref().and_then(|s| s.resolve_for_platform()).unwrap_or_else(|| name.clone());
                    let link = normalize_name(raw_link);

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/package.rs
@@ -701,6 +700,7 @@ repo = "example/codex"
{
assert_eq!(resolved.bin.len(), 3);
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

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

The Windows-only assertions contain a duplicate assert_eq!(resolved.bin.len(), 3); (it appears twice), which is redundant and adds noise to the test. Remove one of them (and keep the one that best matches the comment) to make failures clearer.

Suggested change
assert_eq!(resolved.bin.len(), 3);

Copilot uses AI. Check for mistakes.
@Yangmoooo Yangmoooo deleted the codex/fix-codex-binary-install-linux branch March 23, 2026 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants