Skip to content

ctest: Revise generate function to return Result #4430

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

Merged
merged 1 commit into from
May 13, 2025

Conversation

Owen-CH-Leung
Copy link
Contributor

@Owen-CH-Leung Owen-CH-Leung commented Apr 26, 2025

Description

As per #4369 , the generate function should return a Result instead of returning nothing.

This PR is a continuation of #4424 which was closed due to bad rebase.

Checklist

  • Relevant tests in libc-test/semver have been updated
  • No placeholder or unstable values like *LAST or *MAX are
    included (see rust-lang/libc#3131)
  • Tested locally (cd libc-test && cargo test --target mytarget);
    especially relevant for platforms that may not be checked in CI

@rustbot
Copy link
Collaborator

rustbot commented Apr 26, 2025

r? @tgross35

rustbot has assigned @tgross35.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added ctest Issues relating to the ctest crate S-waiting-on-review labels Apr 26, 2025
@Owen-CH-Leung Owen-CH-Leung marked this pull request as ready for review April 27, 2025 00:39
ctest/src/lib.rs Outdated
use std::collections::{HashMap, HashSet};
use std::env;
use std::fs::File;
use std::io::prelude::*;
use std::io::BufWriter;
use std::panic::{catch_unwind, AssertUnwindSafe};
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately we can't use catch_unwind; if somebody has panic = abort in their Cargo.toml or if run on a target/backend that doesn't support unwinding, the program will stop and catch_unwind won't actually catch anything. All functions that may return an error need to change their signature then update .unwrap() / .expect() to ?.

Everything else looks pretty good to me, but I'll re-review after the changes.

(basically my same comment from #4424 (comment))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I've removed the remaining catch_unwind logic.

Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

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

Looks much better! I left a few more comments.

ctest/src/lib.rs Outdated
Comment on lines 874 to 877
match cfg.try_compile(&format!("lib{stem}.a")) {
Ok(_) => Ok(out),
Err(e) => Err(anyhow::anyhow!("Compilation failed").context(format!("Error: {}", e))),
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the error handling could be a bit simplified here:

let name = format!("lib{stem}.a");
cfg.try_compile(&name)
    .context("failed to compile `{name}`")?
    .map(|()| out)

ctest/src/lib.rs Outdated
Comment on lines 888 to 898
// Check if all specified header files exist in the include paths
for header in &self.headers {
// Check if header exists in any of the include paths
for include_path in &self.includes {
let header_path = include_path.join(header);
if !header_path.exists() {
return Err(anyhow::anyhow!("Header file not found"))
.with_context(|| format!("Path {} does not exist", header_path.display()));
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Where do the header files actually get read? There is a TOCTOU error here (albeit mild). It would be better to just add an error message wherever they are opened, and this block can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. I've removed this block. Thanks!

ctest/src/lib.rs Outdated
Comment on lines 907 to 912
if !out_dir.is_dir() {
return Err(anyhow::anyhow!(
"Output directory does not exist: {}",
out_dir.display()
));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to the above, we shouldn't really need this check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure & removed

ctest/src/lib.rs Outdated
Comment on lines 938 to 941
let diag_info = format!("{:?}", d);
// Emit the diagnostic to properly handle it and show error to the user
d.emit();
anyhow::anyhow!("failed to parse crate").context(format!("Diagnostic: {}", diag_info))
Copy link
Contributor

Choose a reason for hiding this comment

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

            d.emit();
            anyhow!("failed to parse crate: {d:?}")

No need to put the diagnostic in context since it goes with the error, or to format! it separately. Also you can import anyhow::anyhow since it's used a few times.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, why .emit() the diagnostic at all? Does that give different output compared to printing it with either Debug or Display?

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 emit() is necessary becoz without the emit call, the DiagnosticBuilder will panic on drop:

https://github.com/JohnTitor/garando/blob/master/garando_errors/src/diagnostic_builder.rs#L157

@tgross35 tgross35 changed the title Revise generate function to return Result ctest: Revise generate function to return Result May 4, 2025
@Owen-CH-Leung
Copy link
Contributor Author

Owen-CH-Leung commented May 4, 2025

Looks like there is a flaky test :

https://github.com/rust-lang/libc/runs/41604457695

I re-trigger the CI and it passes

Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

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

LGTM! We can update more .unwrap() to proper errors later. Please squash.

@tgross35 tgross35 added the stable-unneeded This PR is applied to main but already exists on libc-0.2 label May 12, 2025
…Add anyhow error handling. Revert expect call in generate API

Add missing impl. rustfmt & clippy

Remove leftovers

Fix linking error due to use of OsString

Remove panic_payload_to_string. Remove catch_unwind

Emit Diagnostic Error before mapping to anyhow

Trigger CI again

Remove header file check and out_dir check

Trigger CI again
@Owen-CH-Leung Owen-CH-Leung force-pushed the add_try_generate_signature branch from 586da1b to c54f1b9 Compare May 13, 2025 10:25
@Owen-CH-Leung
Copy link
Contributor Author

LGTM! We can update more .unwrap() to proper errors later. Please squash.

Done :)

Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

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

Thanks for the work here!

@tgross35 tgross35 added this pull request to the merge queue May 13, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 13, 2025
@tgross35 tgross35 added this pull request to the merge queue May 13, 2025
Merged via the queue into rust-lang:main with commit 76696a2 May 13, 2025
48 of 50 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ctest Issues relating to the ctest crate S-waiting-on-review stable-unneeded This PR is applied to main but already exists on libc-0.2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants