Skip to content
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.

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

incompatibilities with big-endian target platforms / CPU architectures #36

Closed
decathorpe opened this issue Oct 8, 2023 · 2 comments · Fixed by #37
Closed

incompatibilities with big-endian target platforms / CPU architectures #36

decathorpe opened this issue Oct 8, 2023 · 2 comments · Fixed by #37

Comments

@decathorpe
Copy link

I'm working on packaging this crate for Fedora Linux as a dependency of cargo-deny / cargo-audit. I noticed that tests fail on one of our officially supported architectures - the only big-endian architecture we support, s390x / IBM System Z.

From the output of cargo test --release (plus a few more distribution-specific flags):

...
test utils::test::canonicalizes_git_urls ... FAILED
test utils::test::matches_cargo ... FAILED
...
failures:
---- utils::test::canonicalizes_git_urls stdout ----
thread 'utils::test::canonicalizes_git_urls' panicked at src/utils.rs:298:9:
assertion `left == right` failed
  left: "git.luolix.top-016fae53232cc64d"
 right: "git.luolix.top-1b7079c2183ff52e"
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
---- utils::test::matches_cargo stdout ----
thread 'utils::test::matches_cargo' panicked at src/utils.rs:324:9:
assertion `left == right` failed
  left: ("registry/index/git.luolix.top-eae4ba8cbf2ce1c7", "https://github.com/rust-lang/crates.io-index")
 right: ("registry/index/git.luolix.top-1ecc6299db9ec823", "https://github.com/rust-lang/crates.io-index")
failures:
    utils::test::canonicalizes_git_urls
    utils::test::matches_cargo
test result: FAILED. 12 passed; 2 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.01s

From what I can tell, this is caused by this function:
https://github.com/EmbarkStudios/tame-index/blob/0.7.1/src/utils.rs#L18-L37

I think this chunk of code does not produce the same results on little-endian and big-endian architectures, and will produce results that are unexpectedly "shuffled" according to the different byte order:

    const CHARS: &[u8] = b"0123456789abcdef";

    for (i, &byte) in input.iter().enumerate() {
        let i = i * 2;
        output[i] = CHARS[(byte >> 4) as usize];
        output[i + 1] = CHARS[(byte & 0xf) as usize];
    }
@Jake-Shadle
Copy link
Member

This is the same algorithm that cargo uses, is cargo not packaged for that target as well?

u64 hash -> bytes

https://github.com/rust-lang/cargo/blob/0871c0e2f4b6d4f95b7408fb8cef8ad3c9c8d834/src/cargo/util/hex.rs#L7

bytes -> string

https://github.com/KokaKiwi/rust-hex/blob/c333cf5128b6f5135d8f561b217f68e670275031/src/lib.rs#L106-L112

I can of course fix this so it is consistent for big endian targets, but AFAICT the directory will be different on big endian machines, according to cargo's own tests

https://github.com/rust-lang/cargo/blob/0871c0e2f4b6d4f95b7408fb8cef8ad3c9c8d834/src/cargo/core/source_id.rs#L930-L936

I think the best option is to to do the same as cargo and ignore the tests on big endian machines, the tests are just to verify we match cargo, if they pass on little endian they will also match on big endian.

@decathorpe
Copy link
Author

Oh, of course, if cargo does the exact same thing, then the results will match (even if they are different depending on architecture endianness) ... thanks for the clarification and the "fix" :) 👍🏼

mergify bot pushed a commit to EmbarkStudios/cargo-deny that referenced this issue Nov 7, 2023
The hexadecimal part of cargo index paths is different on little- and
big-endian architectures.
See also EmbarkStudios/tame-index#36

I have verified that the endianness-specific `CRATES_IO_SPARSE_DIR`
definition is correct and indeed matches what cargo writes to on
big-endian architectures.
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 a pull request may close this issue.

2 participants