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.

Sign up for GitHub

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

Jump to bottom

Use SHA256 source file checksums by default when targeting MSVC #113707

Merged
merged 1 commit into from
Jul 21, 2023
Merged

Use SHA256 source file checksums by default when targeting MSVC #113707

merged 1 commit into from
Jul 21, 2023

Conversation

sivadeilra
Copy link

Currently, when targeting Windows (more specifically, the MSVC toolchain), Rust will use SHA1 source file checksums by default. SHA1 has been superseded by SHA256, and Microsoft recommends migrating to SHA256.

As of Visual Studio 2022, MSVC defaults to SHA256. This change aligns Rust and MSVC.

LLVM can already use SHA256 checksums, so this does not require any change to LLVM.

MSVC docs on source file checksums: https://learn.microsoft.com/en-us/cpp/build/reference/zh?view=msvc-170

@rustbot
Copy link
Collaborator

rustbot commented Jul 14, 2023

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

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 14, 2023
@jyn514
Copy link
Member

jyn514 commented Jul 15, 2023

why does this use a different hash algorithm on windows? sha256 seems fine - shouldn't we use that for all platforms?

@sivadeilra
Copy link
Author

I don't know. Maybe the debuggers on non- Windows platforms don't all support SHA256 yet.

@fee1-dead
Copy link
Member

r? compiler

@rustbot rustbot assigned eholk and unassigned fee1-dead Jul 18, 2023
@wesleywiser
Copy link
Member

The DWARF 5 spec only has support for MD5 (section 6.2.4.1 or search for DW_LNCT_MD5). It's possible gdb/lldb/etc have non-standard extensions to support better algorithms but I don't believe they do.

@eholk
Copy link
Contributor

eholk commented Jul 18, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Jul 18, 2023

📌 Commit fcdff63 has been approved by eholk

It is now in the queue for this repository.

@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 Jul 18, 2023
@GuillaumeGomez GuillaumeGomez mentioned this pull request Jul 19, 2023
Closed
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jul 19, 2023
@GuillaumeGomez
Rollup merge of rust-lang#113707 - sivadeilra:user/ardavis/sha256, r=…
5739a5c 
…eholk

Use SHA256 source file checksums by default when targeting MSVC

Currently, when targeting Windows (more specifically, the MSVC toolchain), Rust will use SHA1 source file checksums by default.  SHA1 has been superseded by SHA256, and Microsoft recommends migrating to SHA256.

As of Visual Studio 2022, MSVC defaults to SHA256.  This change aligns Rust and MSVC.

LLVM can already use SHA256 checksums, so this does not require any change to LLVM.

MSVC docs on source file checksums: https://learn.microsoft.com/en-us/cpp/build/reference/zh?view=msvc-170
@bors
Copy link
Contributor

bors commented Jul 20, 2023

⌛ Testing commit fcdff63 with merge 0642dbf7103f39f6144730c05954af88958e3889...

@wesleywiser
Copy link
Member

Bors seems to have forgotten about this?

@bors r=eholk

@bors
Copy link
Contributor

bors commented Jul 20, 2023

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Contributor

bors commented Jul 20, 2023

📌 Commit fcdff63 has been approved by eholk

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jul 21, 2023

⌛ Testing commit fcdff63 with merge 5419aa3...

@bors
Copy link
Contributor

bors commented Jul 21, 2023

☀️ Test successful - checks-actions
Approved by: eholk
Pushing 5419aa3 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 21, 2023
@bors bors merged commit 5419aa3 into rust-lang:master Jul 21, 2023
@rustbot rustbot added this to the 1.73.0 milestone Jul 21, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (5419aa3): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -3.1% [-3.1%, -3.1%] 1
All ❌✅ (primary) - - 0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 651.312s -> 651.291s (-0.00%)

@sivadeilra sivadeilra deleted the user/ardavis/sha256 branch July 21, 2023 15:23
@klensy klensy mentioned this pull request Sep 22, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@eholk eholk

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.73.0
Development

Successfully merging this pull request may close these issues.
8 participants
@sivadeilra @rustbot @jyn514 @fee1-dead @wesleywiser @eholk @bors @rust-timer