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

[nix] fix add-determinsim and mill-deps building on darwin #62

Merged
merged 6 commits into from
Feb 2, 2025

Conversation

Emin017
Copy link

@Emin017 Emin017 commented Jan 25, 2025

  • bump add-determinism to unstable-2024-11-12 and add patches for darwin
  • fix mill-deps building on darwin (The caches dir on darwin is $TMPDIR/Library/Caches/Coursier)

@Emin017 Emin017 changed the title [nix] fix add-determinsim and mill-deps building on macOS [nix] fix add-determinsim and mill-deps building on darwin Jan 25, 2025
* bump add-determinism to unstable-2024-11-12 and add patches for darwin
* fix mill-deps building on darwin (The caches dir on darwin is
`$TMPDIR/Library/Caches/Coursier`)
@Avimitin
Copy link
Contributor

I would suggest sending a PR to upstream for the darwin fix, instead of maintaining patch in this repository.

@Emin017
Copy link
Author

Emin017 commented Jan 31, 2025

I would suggest sending a PR to upstream for the darwin fix, instead of maintaining patch in this repository.

Waiting for keszybz/add-determinism#48

@Avimitin
Copy link
Contributor

Thanks for your upstream efforts. I think now we can use fetchpatch and remove it after upstream merge this.

@Emin017
Copy link
Author

Emin017 commented Jan 31, 2025

Thanks for your upstream efforts. I think now we can use fetchpatch and remove it after upstream merge this.

I removed patch in the new commit, but I think after nixpkgs upstream merge nixpkgs#376957, we can remove add-determinism package here.

@Avimitin
Copy link
Contributor

Avimitin commented Feb 1, 2025

Please fix the CI.

@Emin017
Copy link
Author

Emin017 commented Feb 1, 2025

It seems that the outputHash of chisel publishMillModule obtained through the publishLocal method is different each time :(

@sequencer
Copy link
Member

🫠

@Avimitin
Copy link
Contributor

Avimitin commented Feb 1, 2025

It seems that the outputHash of chisel publishMillModule obtained through the publishLocal method is different each time :(

Yeah and that's why we need fix determinism. I will handle this.

@Avimitin
Copy link
Contributor

Avimitin commented Feb 1, 2025

This should work. I can't commit changes directly to your fork, so try git apply the following patch.

diff --git a/templates/chisel/nix/pkgs/dependencies/default.nix b/templates/chisel/nix/pkgs/dependencies/default.nix
index b8c8d38..bd9afa2 100644
--- a/templates/chisel/nix/pkgs/dependencies/default.nix
+++ b/templates/chisel/nix/pkgs/dependencies/default.nix
@@ -1,6 +1,7 @@
 { pkgs
 , stdenv
 , mill
+, add-determinism
 , ...
 }:
 { name
@@ -42,6 +43,10 @@ stdenv.mkDerivation {
     runHook preInstall
     mkdir -p $out/.ivy2
     mv $TMPDIR/ivy/local $out/.ivy2/local
+
+    export SOURCE_DATE_EPOCH=1669810380
+    find $out -type f -name '*.jar' -exec '${add-determinism}/bin/add-determinism' -j "$NIX_BUILD_CORES" '{}' ';'
+
     runHook postInstall
   '';
 }

@Emin017
Copy link
Author

Emin017 commented Feb 1, 2025

This should work. I can't commit changes directly to your fork, so try git apply the following patch.

diff --git a/templates/chisel/nix/pkgs/dependencies/default.nix b/templates/chisel/nix/pkgs/dependencies/default.nix
index b8c8d38..bd9afa2 100644
--- a/templates/chisel/nix/pkgs/dependencies/default.nix
+++ b/templates/chisel/nix/pkgs/dependencies/default.nix
@@ -1,6 +1,7 @@
 { pkgs
 , stdenv
 , mill
+, add-determinism
 , ...
 }:
 { name
@@ -42,6 +43,10 @@ stdenv.mkDerivation {
     runHook preInstall
     mkdir -p $out/.ivy2
     mv $TMPDIR/ivy/local $out/.ivy2/local
+
+    export SOURCE_DATE_EPOCH=1669810380
+    find $out -type f -name '*.jar' -exec '${add-determinism}/bin/add-determinism' -j "$NIX_BUILD_CORES" '{}' ';'
+
     runHook postInstall
   '';
 }

Thanks for you help, I realize I have very little knowledge about the use of add-determinism.
I will push the modified version.

@Emin017
Copy link
Author

Emin017 commented Feb 1, 2025

This hash value was able to complete the build successfully on another mac, I think we might need to differentiate the hash value for the darwin platform. I'm trying to test it on another x86 linux machine.

@Emin017
Copy link
Author

Emin017 commented Feb 1, 2025

The hash value I get after building on my local machine is different from the hash value I get on my European servers, do we still need to use determinism for certain jar files?

@Avimitin
Copy link
Contributor

Avimitin commented Feb 1, 2025

I think mill might encode some machine specific information to the buikd output. We need to unpack the JAR archive and see what is different.

@Avimitin
Copy link
Contributor

Avimitin commented Feb 1, 2025

It seems like the hash mismatch is from mill deps instead of publishMillJar

@Emin017
Copy link
Author

Emin017 commented Feb 1, 2025

I pushed a new commit and it should work, but I think there is still a need to dig deeper into why there are different hash values on different machines? (I tested on my servers, and even two x86_64 linux machines will have a hash mismatch)

I'm still not very familiar with offline dependency builds for mill, but I'm happy to help if there's anything I can do here

@Avimitin
Copy link
Contributor

Avimitin commented Feb 1, 2025

You can build the two hash mismatched derivation and runs diff -bur on two derivation paths to see what is different. For jar file, you can use the unzip tool to extract its content, for example, unzip chisel.jar.

@Emin017
Copy link
Author

Emin017 commented Feb 1, 2025

Thank you for your advice. I will try to find the problem.

Chisel will generate lowercase directoreise by default now, see:
chipsalliance/chisel#4505.

Signed-off-by: Qiming Chu <[email protected]>
@Avimitin
Copy link
Contributor

Avimitin commented Feb 2, 2025

Seems like that you got invalid mill deps downloaded.

@Emin017
Copy link
Author

Emin017 commented Feb 2, 2025

Seems like that you got invalid mill deps downloaded.

It seems there may be a network issue with the GitHub runner. I can run this Check Scala format CI tests successfully on my machine, and I can also find the corresponding download files on the Maven repositories.

@Avimitin
Copy link
Contributor

Avimitin commented Feb 2, 2025

Is it ready to merge now?

@Emin017
Copy link
Author

Emin017 commented Feb 2, 2025

I think we can go ahead and merge this PR. I'm still comparing the differences in mill output between machines, and if I'm able to resolve the issue, I can fix it in a new PR.

@Avimitin Avimitin merged commit 25f99f4 into chipsalliance:mill-bump Feb 2, 2025
5 checks passed
@Avimitin
Copy link
Contributor

Avimitin commented Feb 2, 2025

Thx for your contribution!

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.

3 participants