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

rust-cbindgen: use merged upstream patch #352102

Closed
wants to merge 1 commit into from

Conversation

bryango
Copy link
Member

@bryango bryango commented Oct 29, 2024

... instead of vendoring.

Note: targeted staging-next but only tested on top of master.

This is a follow-up of commit: 27c05ca, in which we vendored the patch since upstream was unresponsive at that time. Now that the patch has been merged in:

... it would be better for us to vendor the patch (which is improved with upstream feedback).

I am targeting staging-next but if it's not appropriate, just let me know and I can re-target! P.S. The fetched patch in plain text here (nix build --file . rust-cbindgen.patches; cat result):

diff --git a/tests/profile.rs b/tests/profile.rs
index 69433a2b..0e7eba7d 100644
--- a/tests/profile.rs
+++ b/tests/profile.rs
@@ -1,6 +1,7 @@
 use cbindgen::*;
 
 use serial_test::serial;
+use std::env;
 use std::path::{Path, PathBuf};
 use std::process::Command;
 
@@ -17,7 +18,12 @@
         .tempdir()
         .expect("Creating tmp dir failed");
 
-    std::env::set_var("CARGO_EXPAND_TARGET_DIR", tmp_dir.path());
+    unsafe {
+        env::set_var("CARGO_EXPAND_TARGET_DIR", tmp_dir.path());
+        env::remove_var("CARGO_BUILD_TARGET");
+        // ^ avoid unexpected change of layout of the target directory;
+        // ... see: https://doc.rust-lang.org/cargo/guide/build-cache.html
+    }
     let builder = Builder::new()
         .with_config(Config::from_file(expand_dep_test_dir.join("cbindgen.toml")).unwrap())
         .with_crate(expand_dep_test_dir);
@@ -45,6 +51,9 @@
     Command::new(cbindgen_path)
         .current_dir(expand_dep_test_dir)
         .env("CARGO_EXPAND_TARGET_DIR", tmp_dir.path())
+        .env_remove("CARGO_BUILD_TARGET")
+        // ^ avoid unexpected change of layout of the target directory;
+        // ... see: https://doc.rust-lang.org/cargo/guide/build-cache.html
         .args(extra_args)
         .output()
         .expect("build should succeed");
@@ -87,6 +96,16 @@
     assert_eq!(get_contents_of_dir(target_dir.path()), &["debug"]);
 }
 
+#[test]
+#[serial]
+fn bin_ignore_cargo_build_target_in_tests() {
+    unsafe {
+        env::set_var("CARGO_BUILD_TARGET", "x86_64-unknown-linux-gnu");
+    }
+    // ^ this env var should be ignored:
+    bin_default_uses_debug_build();
+}
+
 #[test]
 fn bin_explicit_debug_build() {
     let target_dir = build_using_bin(&["--profile", "debug"]);

Build log on top of master: https://github.com/bryango/nix-build-action/actions/runs/11574144911/job/32217761733

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

Sorry, something went wrong.

... instead of vendoring. This is a follow-up of commit:

  27c05ca
@bryango bryango requested review from emilazy and alyssais October 29, 2024 12:52
@alyssais
Copy link
Member

Is it worth it changing again, or should we just wait for the next release?

@bryango
Copy link
Member Author

bryango commented Oct 29, 2024

Is it worth it changing again, or should we just wait for the next release?

Indeed, also we don't have to do this in this staging cycle (if the previous fix already works). Upstream releases are rather sparse though; see https://github.com/mozilla/cbindgen/releases.

@emilazy
Copy link
Member

emilazy commented Oct 29, 2024

If we could encourage upstream to make a release, that would be good for other downstreams who might be affected by the bug too.

@bryango
Copy link
Member Author

bryango commented Oct 30, 2024

If we could encourage upstream to make a release, that would be good for other downstreams who might be affected by the bug too.

I have sent the query upstream! If the next release is imminent, we can just wait and then bump; if it's far away, we can merge this PR or retarget staging for the next staging cycle.

@mweinelt
Copy link
Member

New release #374202

@mweinelt mweinelt closed this Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants