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

Fix test: hash value depends on endianness and bitness. #10011

Merged
merged 3 commits into from
Nov 10, 2021

Conversation

hkratz
Copy link
Contributor

@hkratz hkratz commented Oct 25, 2021

The test fails on 32-bit systems and on big-endian systems since Rust 1.44.

Fixes #10004.

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 25, 2021
@alexcrichton
Copy link
Member

Could this test be updated to only run on little-endian 64-bit machines, aka what we're running in CI? Most of us don't have access to all these machines to reproduce the hashes and it's intended for us as developers of Cargo really and not much else.

@alexcrichton
Copy link
Member

Could you add a comment as well as to why this test only runs on some platforms?

Hash value is different depenidng on bitness and endianess so we
only run this test on 64-bit little endian platforms.
// The hash value differs depending on endianess and bit-width since Rust 1.45
// (see https://github.com/rust-lang/rust/issues/74215). We run this test only
// on 64-bit little-endian platforms which are most commonly used for
// development and tested in CI.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the bit about 1.45 or linking to a Rust issue is really all that relevant here, I think the comment can basically say that the hash value here is machine-specific, so this test is limited to run only on machines where the listed value is actually correct. This platform happens to correspond to x86_64 which is used for CI which means we should see failures.

Copy link
Contributor

Choose a reason for hiding this comment

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

The hash-value is machine-specific because of that issue. I think it's not representative of "the whole picture" to say that this test behaviour is "intended for us as developers of Cargo really and not much else." - the hash is used on end-users' filesystems as part of a file path. This is not machine-dependent e.g. when home directories are mounted via NFS and shared across different machines e.g. in a university network.

It may not be a big deal in this particular case, at the very least it will result in some duplication, but I think it's important to understand what the consequences are in the wider context of general computing environments rather than wave it away as a "developer-only" issue.

Stable hashing is a really nice thing to have for protocols and other situations where you want to interoperate, and it's a shame if the Rust standard library wouldn't support it.

Copy link
Contributor

@infinity0 infinity0 Oct 25, 2021

Choose a reason for hiding this comment

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

This comment in src/cargo/core/compiler/fingerprint.rs suggests that StableHasher was originally intended to be platform-independent (as would be a reasonable assumption from the naming):

//! Note: Fingerprinting is not a perfect solution. Filesystem mtime tracking
//! is notoriously imprecise and problematic. Only a small part of the
//! environment is captured. This is a balance of performance, simplicity, and
//! completeness. Sandboxing, hashing file contents, tracking every file
//! access, environment variable, and network operation would ensure more
//! reliable and reproducible builds at the cost of being complex, slow, and
//! platform-dependent.

Copy link
Member

Choose a reason for hiding this comment

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

Ok well in that case I'll leave this to the tagged reviewer.

Copy link
Contributor Author

@hkratz hkratz Oct 26, 2021

Choose a reason for hiding this comment

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

Ok well in that case I'll leave this to the tagged reviewer.

@alexcrichton Oh, I did not see that reply when commenting (below). 😦

@joshtriplett
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Nov 10, 2021

📌 Commit aa00def has been approved by joshtriplett

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 10, 2021
@bors
Copy link
Collaborator

bors commented Nov 10, 2021

⌛ Testing commit aa00def with merge e11cd81...

@bors
Copy link
Collaborator

bors commented Nov 10, 2021

☀️ Test successful - checks-actions
Approved by: joshtriplett
Pushing e11cd81 to master...

@bors bors merged commit e11cd81 into rust-lang:master Nov 10, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 18, 2021
Update cargo

11 commits in 2e2a16e983f597da62bc132eb191bc3276d4b1bb..ad50d0d266213e0cc4f6e526a39d96faae9a3842
2021-11-08 15:13:38 +0000 to 2021-11-17 18:36:37 +0000
- Warn when alias shadows external subcommand (rust-lang/cargo#10082)
- Implement escaping to allow clean -p to delete all files when directory contains glob characters (rust-lang/cargo#10072)
- Match any error when failing to find executables (rust-lang/cargo#10092)
- Enhance error message for target auto-discovery (rust-lang/cargo#10090)
- Include note about bug while building on macOS in mdbook (rust-lang/cargo#10073)
- Improve the help text of the --quiet args for all commands (rust-lang/cargo#10080)
- `future-incompat-report` checks both stdout and stderr for color support (rust-lang/cargo#10024)
- Remove needless borrow to make clippy happy (rust-lang/cargo#10081)
- Describe the background color of the timing graph (rust-lang/cargo#10076)
- Make ProfileChecking comments a doc comments (rust-lang/cargo#10077)
- Fix test: hash value depends on endianness and bitness. (rust-lang/cargo#10011)
@ehuss ehuss added this to the 1.58.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test_cratesio_hash fails on systems that are not little-endian 64-bit
7 participants