Revise generate function to return Result #4424

Owen-CH-Leung · 2025-04-19T04:46:52Z

Description

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

Checklist

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

…t/build.rs to test out the error handling logic

rustbot · 2025-04-19T04:46:57Z

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

Fix an `unnecessary_transmutes` from a recent nightly

netbsd move from e! marcro to c_enum!

run `cargo clippy --all-targets` on `libc-test/build.rs`, and fix all default issues. ### Notes * `copy_dir_hotfix` had a `replace` parameter that was never used

run `cargo clippy --all-targets` on `ctest-test`, and fix all default issues. Also added a generic

linux: add constant PACKET_IGNORE_OUTGOING

chore: lint `libc-test/build.rs`

chore: lint `ctest-test` crate

Fix querying emcc on windows

Update pidfd constants and types (Linux 6.9-6.15)

chore: apply some clippy lints

tgross35

Thank you for picking this up!

The results of the new tests look good, but unfortunately the internal functions also need to be updated to return Result rather than just catching panics.

tgross35 · 2025-04-25T07:26:14Z

ctest-test/build.rs

+    let orig_out_dir = std::env::var_os("OUT_DIR");
+    std::env::remove_var("OUT_DIR");
+
+    let err1 = ctest::TestGenerator::new()
+        .header("t1.h")
+        .generate("src/t1.rs", "out_dir_gen.rs");
+
+    // Restore OUT_DIR
+    if let Some(dir) = orig_out_dir {
+        std::env::set_var("OUT_DIR", dir);
+    }
+
+    assert!(err1.is_err(), "Expected error when OUT_DIR is missing");
+
+    // Test error handling for invalid output path
+    let err = ctest::TestGenerator::new()
+        .header("t1.h")
+        .include("src")
+        .out_dir("/nonexistent_dir") // Should fail with permission error
+        .generate("src/t1.rs", "out_path_gen.rs");
+
+    assert!(err.is_err(), "Expected error with invalid output path");
+
+    // Test parsing error
+    // Create a temporary file with invalid Rust syntax
+    let temp_dir = env::temp_dir();
+    let invalid_file = temp_dir.join("invalid.rs");
+    std::fs::write(&invalid_file, "fn invalid_syntax {").unwrap();
+
+    let err = ctest::TestGenerator::new()
+        .header("t1.h")
+        .include("src")
+        .generate(&invalid_file, "parse_gen.rs");
+
+    assert!(err.is_err(), "Expected error when parsing invalid syntax");
+    let _ = std::fs::remove_file(invalid_file);
+
+    // Test non-existent header
+    let err = ctest::TestGenerator::new()
+        .header("nonexistent_header.h")
+        .include("src")
+        .generate("src/t1.rs", "missing_header_gen.rs");
+
+    assert!(err.is_err(), "Expected error with non-existent header");
+
+    // Test invalid include path
+    let err = ctest::TestGenerator::new()
+        .header("t1.h")
+        .include("nonexistent_directory")
+        .generate("src/t1.rs", "invalid_include_gen.rs");
+
+    assert!(err.is_err(), "Expected error with invalid include path");


build.rs works for the test that runs the whole crate, but these assert! tests should be able to work on their own. Could you move them to ctest-test/tests/all.rs?

Thanks for your review! I'll file a commit to move them to ctest-test/tests/all.rs soon.

tgross35 · 2025-04-25T07:29:10Z

ctest/src/lib.rs

@@ -793,9 +795,9 @@ impl TestGenerator {
    /// let mut cfg = TestGenerator::new();
    /// cfg.generate("../path/to/libfoo-sys/lib.rs", "all.rs");
    /// ```
-    pub fn generate<P: AsRef<Path>>(&mut self, krate: P, out_file: &str) {
+    pub fn generate<P: AsRef<Path>>(&mut self, krate: P, out_file: &str) -> Result<PathBuf> {


It would be best if we could do this in a way that doesn't break semver, just because this repo currently has a tricky branch setup. Could generate keep its signature, but move the body to a new fn try_generate(...) -> Result? The existing generate can just call try_generate and then panic if there is a failure.

Sure. I'll keep the generate function signature and move them to try_generate instead

tgross35 · 2025-04-25T07:32:23Z

ctest/src/lib.rs

I think it would be easiest to switch our Error to anyhow's Error. This gives us .context() and .with_context() which should make using ? a lot easier.

Ah ok. Yeah I think using anyhow's Error is a more idiomatic way to handle error. I'll add anyhow as part of dependencies and make the code change soon

tgross35 · 2025-04-25T07:36:35Z

ctest/src/lib.rs

+        let stem = out
+            .file_stem()
+            .ok_or_else(|| Error::from("Output file has no stem"))?
+            .to_str()
+            .ok_or_else(|| Error::from("Output filename is not valid UTF-8"))?;
+
+        let parent = out
+            .parent()
+            .ok_or_else(|| Error::from("Output file has no parent directory"))?;


After switching to anyhow this will be a bit easier to use .context and ?

tgross35 · 2025-04-25T07:39:29Z

ctest/src/lib.rs

+
+        cfg.target(&target).out_dir(parent);
+
+        match catch_unwind(|| cfg.compile(&format!("lib{stem}.a"))) {


Unfortunately not quite :) we want to change all internal functions to return a Result as well so it's easier to know what might fail and for simpler testing.

It should be pretty easy to find+replace any .unwrap() with ?, then update the function signatures anywhere it complains.

Update Redox SA_ constants.

…H-Leung/libc into revise_generate_signature

…Add anyhow error handling. Revert expect call in generate API

rustbot · 2025-04-26T14:11:36Z

Some changes occurred in solarish module

cc @jclulow, @pfmooney

Some changes occurred in the Android module

cc @maurer

Some changes occurred in OpenBSD module

cc @semarie

Owen-CH-Leung · 2025-04-26T14:12:42Z

Sorry - bad rebase here. Will close this MR and create a new one

rusty-snake and others added 2 commits April 18, 2025 10:50

Update pidfd constants and types (Linux 6.9-6.15)

126f2c6

Revise generate function to return Result. Add unit test in ctest-tes…

fdf9b3e

…t/build.rs to test out the error handling logic

rustbot assigned tgross35 Apr 19, 2025

rustbot added ctest Issues relating to the ctest crate S-waiting-on-review labels Apr 19, 2025

Owen-CH-Leung changed the title ~~Revise generate function to return Result. Add unit test in ctest-tes…~~ Revise generate function to return Result Apr 19, 2025

Owen-CH-Leung added 2 commits April 19, 2025 16:54

Remove use of backtrace. Fix clippy CI

dcb8904

Refactor generate_file_impl

1b24825

Owen-CH-Leung marked this pull request as ready for review April 19, 2025 12:25

devnexen and others added 15 commits April 19, 2025 22:16

netbsd move from e! marcro to c_enum!

f92ed6f

Fix an unnecessary_transmutes from a recent nightly

673af93

Merge pull request rust-lang#4429 from tgross35/lint-fixes

d00adf4

Fix an `unnecessary_transmutes` from a recent nightly

Merge pull request rust-lang#4425 from devnexen/netbsd_e_to_c_enum

00e6d6a

netbsd move from e! marcro to c_enum!

chore: lint libc-test/build.rs

8b15e27

run `cargo clippy --all-targets` on `libc-test/build.rs`, and fix all default issues. ### Notes * `copy_dir_hotfix` had a `replace` parameter that was never used

linux: add constant PACKET_IGNORE_OUTGOING

29ab31e

chore: lint ctest-test crate

4a88460

run `cargo clippy --all-targets` on `ctest-test`, and fix all default issues. Also added a generic

Merge pull request rust-lang#4319 from ryancdotorg/main

59a0f83

linux: add constant PACKET_IGNORE_OUTGOING

Merge pull request rust-lang#4412 from nyurik/libc-test-lints

ec7da63

chore: lint `libc-test/build.rs`

Fix querying emcc on windows (use emcc.bat)

1606561

Merge pull request rust-lang#4413 from nyurik/ctest-test-lints

8c10293

chore: lint `ctest-test` crate

chore: apply some clippy lints

a283b9e

Merge pull request rust-lang#4248 from shards-lang/fix-emcc-main

393e909

Fix querying emcc on windows

Merge pull request rust-lang#4402 from rusty-snake/pidfd-threaded

1b5abd7

Update pidfd constants and types (Linux 6.9-6.15)

Merge pull request rust-lang#4415 from nyurik/lints

172b20a

chore: apply some clippy lints

tgross35 requested changes Apr 25, 2025

View reviewed changes

rustbot added S-waiting-on-author and removed S-waiting-on-review labels Apr 25, 2025

tgross35 added the stable-unneeded This PR is applied to main but already exists on libc-0.2 label Apr 25, 2025

Update Redox SA_ constants.

74c3cc0

tgross35 and others added 3 commits April 25, 2025 16:44

Merge pull request rust-lang#4426 from 4lDO2/redox-signal-defs

79a9c77

Update Redox SA_ constants.

Merge branch 'revise_generate_signature' of https://github.com/Owen-C…

0a9e350

…H-Leung/libc into revise_generate_signature

Move test on TestGenerator to all.rs. Move generate to try_generate. …

cfe1679

…Add anyhow error handling. Revert expect call in generate API

rustbot added O-android O-arm O-bsd O-dragonfly O-gnu O-linux O-linux-like O-macos O-mips O-musl O-redox O-riscv O-solarish O-unix O-x86 labels Apr 26, 2025

Owen-CH-Leung closed this Apr 26, 2025

Owen-CH-Leung mentioned this pull request Apr 26, 2025

ctest: Revise generate function to return Result #4430

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revise generate function to return Result #4424

Revise generate function to return Result #4424

Owen-CH-Leung commented Apr 19, 2025

rustbot commented Apr 19, 2025

tgross35 left a comment

tgross35 Apr 25, 2025

Owen-CH-Leung Apr 26, 2025

tgross35 Apr 25, 2025

Owen-CH-Leung Apr 26, 2025

tgross35 Apr 25, 2025

Owen-CH-Leung Apr 26, 2025

tgross35 Apr 25, 2025

tgross35 Apr 25, 2025

rustbot commented Apr 26, 2025

Owen-CH-Leung commented Apr 26, 2025


		cfg.target(&target).out_dir(parent);

		match catch_unwind(\|\| cfg.compile(&format!("lib{stem}.a"))) {

Revise generate function to return Result #4424

Revise generate function to return Result #4424

Conversation

Owen-CH-Leung commented Apr 19, 2025

Description

Checklist

rustbot commented Apr 19, 2025

tgross35 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rustbot commented Apr 26, 2025

Owen-CH-Leung commented Apr 26, 2025