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

filesystem-related nondeterminism from libgit2-sys #618

Closed
bmwiedemann opened this issue Sep 9, 2020 · 3 comments · Fixed by #619
Closed

filesystem-related nondeterminism from libgit2-sys #618

bmwiedemann opened this issue Sep 9, 2020 · 3 comments · Fixed by #619

Comments

@bmwiedemann
Copy link

While working on reproducible builds for openSUSE, I found that
when building below packages, there were slight differences between each build:

  • bat
  • cargo-c
  • exa
  • onefetch

from libgit2-sys
e.g. around git_attr_get_many_with_session.cold

My analysis showed that it is from filesystem-ordering.

I see ordering issues in the build env
/home/abuild/rpmbuild/BUILD/bat-0.15.4/target/release/build/libgit2-sys-c2cd739238a5d529/output
where .h files change order.

@alexcrichton
Copy link
Member

Is this a bug with libgit2? Or build.rs? Sorry but it's not clear to me what this is a bug for at this moment

@kpcyrd
Copy link
Contributor

kpcyrd commented Sep 11, 2020

@bmwiedemann I've very briefly looked into this (without actually reproducing it yet), can you try if this patch improves anything:

diff --git a/libgit2-sys/build.rs b/libgit2-sys/build.rs
index 178d931..f0dbbca 100644
--- a/libgit2-sys/build.rs
+++ b/libgit2-sys/build.rs
@@ -1,5 +1,6 @@
 use std::env;
 use std::fs;
+use std::io;
 use std::path::{Path, PathBuf};
 use std::process::Command;
 
@@ -199,8 +200,10 @@ fn cp_r(from: impl AsRef<Path>, to: impl AsRef<Path>) {
 }
 
 fn add_c_files(build: &mut cc::Build, path: impl AsRef<Path>) {
-    for e in path.as_ref().read_dir().unwrap() {
-        let e = e.unwrap();
+    let dir = path.as_ref().read_dir().unwrap();
+    let mut paths = dir.collect::<io::Result<Vec<_>>>().unwrap();
+    paths.sort_by_key(|e| e.path());
+    for e in paths {
         let path = e.path();
         if e.file_type().unwrap().is_dir() {
             // skip dirs for now

@bmwiedemann
Copy link
Author

@kpcyrd I tested the patch on our bat package and it became reproducible. Thanks. Do you want to open the PR?

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 a pull request may close this issue.

3 participants