Skip to content

Bootstrap the rustc-stable-hash crate #1

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 3 commits into from
Jun 19, 2024

Conversation

Urgau
Copy link
Member

@Urgau Urgau commented Jun 4, 2024

This PR create the rustc-stable-hash crate and import the StableHasher (struct), StableHasherResult (trait) and SipHasher128 (struct) from rustc_data_structures along with their respective tests. All of those should be removed from rustc when it starts using this crate.

This PR also setup a very minimal CI (copied from rustc-hash), it will need to more coverage, it particular of big endian targets.

Also well as importing the Code of Conduct and licenses files.

r? @WaffleLapkin

@michaelwoerister
Copy link
Member

Has this been discussed somewhere? I think extracting something into a separate repo warrants an MCP, if just for visibility.

@Urgau
Copy link
Member Author

Urgau commented Jun 5, 2024

@michaelwoerister There was the team PR rust-lang/team#1472, but apart from that, not really. Only some private discussions.
As asked I created an MCP, rust-lang/compiler-team#755.

@michaelwoerister
Copy link
Member

Thanks, @Urgau!

Copy link
Member

@programmerjake programmerjake Jun 5, 2024

Choose a reason for hiding this comment

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

you should probably test a big-endian target too, e.g. powerpc-unknown-linux-gnu. project-portable-simd uses cross for testing that kind of stuff:
https://github.com/rust-lang/portable-simd/blob/7cd6f95a13d742e482e3b4070d2c01e7b8d8bbfd/.github/workflows/ci.yml

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we should. I mentioned in the PR description that I plan to expand the CI/targets in a follow-up PR.
So as to keep this is PR minimal and focused on just bootstraping the crate.

The portable-simd CI seems like a fairly good starting point. Thanks for pointing that out.

Copy link
Member

Choose a reason for hiding this comment

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

Since the tests are not computationally expensive, miri might also be a viable choice for testing a big endian platform.

Copy link
Member Author

Choose a reason for hiding this comment

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

The currently (limited) CI already uses Miri to test a 32 bit target (i686-unknown-linux-gnu), so it's totally an option.

Copy link
Member Author

Choose a reason for hiding this comment

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

I completely reworked the CI. It is now more extensive.

It now test nightly/stable on Linux/MacOS/Windows.
As well as 32-bits/64-bits on little-endian/big-endian targets with Miri.

Copy link

Choose a reason for hiding this comment

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

CI looks great!

@fbstj
Copy link

fbstj commented Jun 5, 2024

stupid question: does this repository need to have the history of the existing impl from rust-lang/rust imported? or at least mabye linked to (like "this is a copy of [this hash] in rust-lang/rust" somehow?

Copy link
Member

@WaffleLapkin WaffleLapkin left a comment

Choose a reason for hiding this comment

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

+1 to #1 (comment).

Maybe a CHANGELOG.md with the following?

# Unreleased

- import stable hasher implementation from rustc ([afurdatiphg](link to commit))

Copy link
Member

@WaffleLapkin WaffleLapkin left a comment

Choose a reason for hiding this comment

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

All my concerns are resolved.

I think this can be merged now. (but the publication and usage in rustc should wait until MCP completes)

@WaffleLapkin WaffleLapkin merged commit cb8e141 into rust-lang:main Jun 19, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants