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: make linkage truly infallible #1390

Merged
merged 1 commit into from
Sep 3, 2024
Merged

fix: make linkage truly infallible #1390

merged 1 commit into from
Sep 3, 2024

Conversation

Gankra
Copy link
Contributor

@Gankra Gankra commented Sep 3, 2024

also some driveby cleanups

also some driveby cleanups
Copy link
Member

@ashleygwilliams ashleygwilliams left a comment

Choose a reason for hiding this comment

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

in general looks good, brought up in discord but does seem strange to group linkage and signing to me, but this might be because i am missing context?

@@ -215,7 +215,7 @@ impl BuildExpectations {
});
linkage
} else {
determine_linkage(src_path, target)?
determine_linkage(src_path, target)
Copy link
Member

Choose a reason for hiding this comment

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

this is the diff i was looking for haha

file_name: Option<String>,
},
#[error("unable to run linkage report for this type of binary")]
LinkageCheckUnsupportedBinary,
Copy link
Member

Choose a reason for hiding this comment

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

i do think maintaining the name of the file would be useful

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The warning all of these flow into handles reporting the filename so it ends up being redundant noise

@Gankra Gankra merged commit bc0f80f into main Sep 3, 2024
16 checks passed
@Gankra Gankra deleted the fix-linkage branch September 3, 2024 16:29
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.

2 participants