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

Add mill build tool #51110

Merged
merged 9 commits into from Dec 3, 2018
Merged

Add mill build tool #51110

merged 9 commits into from Dec 3, 2018

Conversation

scalavision
Copy link
Contributor

Motivation for this change

Mill is a very nice tool for building java and scala projects. It is easier to learn than sbt, maven and gradle for beginners in Scala.

I can be the maintainer of this package, I am also working on several new bioinformatic tools that I will add shortly to nixpkgs. I am still learning nix so please let me know what could be improved! I have mostly looked at other, similar projects to accomplish this derivation.

Thank you for creating Nix and NixOS, it's an awesome project!

Things done
  • [x ] Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • [x ] NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • [ x] Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • [x ] Assured whether relevant documentation is up to date
  • [ x] Fits CONTRIBUTING.md.

${gnused}/bin/sed -i '0,/mill.MillMain/{s|mill.MillMain|mill.MillMain --no-remote-logging|}' $out/bin/mill
'';
meta = with stdenv.lib; {
homepage = http://www.lihaoyi.com/mill;
Copy link
Member

Choose a reason for hiding this comment

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

Might as well be HTTPS?

cp ${src} $out/bin/mill
chmod +x $out/bin/mill
${gnused}/bin/sed -i '0,/java/{s|java|${jre}/bin/java|}' $out/bin/mill
${gnused}/bin/sed -i '0,/mill.MillMain/{s|mill.MillMain|mill.MillMain --no-remote-logging|}' $out/bin/mill
Copy link
Member

Choose a reason for hiding this comment

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

Do you need to manually do gnused like this? It should be in stdenv, shouldn't it?

propagatedBuildInputs = [ jre ] ;
buildInputs = [ makeWrapper gnused ] ;
phases = "installPhase";
installPhase = ''
Copy link
Member

Choose a reason for hiding this comment

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

Indentation seems to be a little wonky

stdenv.mkDerivation rec {
name = "mill-${version}";
version = "0.3.5";
src = fetchurl {
Copy link
Member

Choose a reason for hiding this comment

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

Use fetchFromGitHub here

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 for all your feedback @alyssais !

Lihaoyi has prebuilt the Mill binary on this url:

I am not able to fetch that since it does not have the .tar.gz extension.

I am not sure how easy it is to build this from source, since it is a build tool that is building itself. Would it be ok to keep the fetchurl. Are there any special requirements concerning using binaries like this in nixpkgs?

I've used sbt that is also a build tool in scala as my recipe:

https://github.com/scalavision/nixpkgs/blob/add-mill-build-tool/pkgs/development/tools/build-managers/sbt/default.nix

Copy link
Member

Choose a reason for hiding this comment

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

Aha! I didn't catch that it was a binary. Generally builds from source are vastly preferred in Nixpkgs. Usually binaries are only used when source builds are impossible (eg if it's proprietary, closed-source software).

If it has a dependency on itself, something that, eg ghc and rustc do it to pull in a binary of the previous release, and use that to build the latest one from source.

Probably best to wait for somebody else to weigh in about the binary – personally I'd rather not have binaries in Nixpkgs when it's not absolutely necessary, but that's not my call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.. I agree about binaries :-) I think creating a build using scalac would be quite some work though. I'll wait to see. Thank you very much for helping me out so far! :-)

sha256 = "19ka81f6vjr85gd8cadn0fv0i0qcdspx2skslfksklxdxs2gasf8";
};
propagatedBuildInputs = [ jre ] ;
buildInputs = [ makeWrapper gnused ] ;
Copy link
Member

Choose a reason for hiding this comment

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

Where do you use makeWrapper?

${gnused}/bin/sed -i '0,/java/{s|java|${jre}/bin/java|}' $out/bin/mill
${gnused}/bin/sed -i '0,/mill.MillMain/{s|mill.MillMain|mill.MillMain --no-remote-logging|}' $out/bin/mill
'';
meta = with stdenv.lib; {
Copy link
Member

Choose a reason for hiding this comment

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

You've taken lib as an argument so this can just be with lib.

};
propagatedBuildInputs = [ jre ] ;
buildInputs = [ makeWrapper gnused ] ;
phases = "installPhase";
Copy link
Member

Choose a reason for hiding this comment

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

What's this line for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is used to override all other phases with my own defined installPhase. This will crash on the unpack phase for example, since it is a binary. Maybe there are better ways doing this? I think I took this from the nix pills.

propagatedBuildInputs = [ jre ] ;
buildInputs = [ makeWrapper gnused ] ;
phases = "installPhase";
installPhase = ''
Copy link
Member

Choose a reason for hiding this comment

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

If you're overriding installPhase you should add runHook preInstall and runHook postInstall to your implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do I need to do that also if we only have our own self defined installPhase? I tested it locally and it works, but I guess that is because they are undefined?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, because it allows people to use overrideAttrs and insert their own things without having to override your whole installPhase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

of course! hehe, didn't think about that! Nice :-)

@alyssais
Copy link
Member

We have a standard commit message format. In this case, it should be mill: init at 0.3.5.


propagatedBuildInputs = [ jre ] ;

phases = "installPhase";
Copy link
Member

@veprbl veprbl Nov 27, 2018

Choose a reason for hiding this comment

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

This is not a good way. You are throwing away the standard unpackPhase and fixupPhase.
I think its fine to instead let the standard phases fail here, they should not terminate the build. You can ask to skip some of the phases by setting

stdenv.mkDerivation {
# ...
dontConfigure = true;
dontBuild = true;
# ...
}

Note that dontConfigure doesn't work at the moment, but it's worth to state the intent.

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 again for helping out on this. I've added the flags you mention, I also override the unpackPhase, so there are no errors occuring during the build. Is this an ok way of doing this? Without it, my build did actually stall locally.

I've also removed the sed patching, it was not necessary it seems.

@veprbl
Copy link
Member

veprbl commented Nov 28, 2018

@scalavision
There are still few problems. Using ${src} will hardcode output path of fetchurl into your installPhase, then you can't override mill.overrideAttrs (_: { src = ...; }). The solution is to use $src which is substituted by bash from environment variable.

Other problem is that propagatedBuildInputs doesn't work for nix-env. The solution here is to use makeWrapper.

Finally, meta.platforms depends on what platforms your actual jre implementation can be built for.

I propose following fixes:

diff --git a/pkgs/development/tools/build-managers/mill/default.nix b/pkgs/development/tools/build-managers/mill/default.nix
index 933a7e56cca..97910d22cb3 100644
--- a/pkgs/development/tools/build-managers/mill/default.nix
+++ b/pkgs/development/tools/build-managers/mill/default.nix
@@ -1,4 +1,5 @@
-{ stdenv, fetchurl, jre }:
+{ stdenv, fetchurl, jre, makeWrapper }:
+
 stdenv.mkDerivation rec {
   name = "mill-${version}";
   version = "0.3.5";
@@ -8,11 +9,9 @@ stdenv.mkDerivation rec {
     sha256 = "19ka81f6vjr85gd8cadn0fv0i0qcdspx2skslfksklxdxs2gasf8";
   };

-  propagatedBuildInputs = [ jre ] ;
+  nativeBuildInputs = [ makeWrapper ];

-  unpackPhase = ''
-    # Skipping unpackphase, nothing to unpack
-  '';
+  unpackPhase = "true";

   dontConfigure = true;

@@ -20,9 +19,9 @@ stdenv.mkDerivation rec {

   installPhase = ''
     runHook preInstall
-    mkdir -p $out/bin
-    cp ${src} $out/bin/mill
-    chmod +x $out/bin/mill
+    install -Dm555 "$src" "$out/bin/.mill-wrapped"
+    # can't use wrapProgram because it sets --argv0
+    makeWrapper "$out/bin/.mill-wrapped" "$out/bin/mill" --prefix PATH : ${stdenv.lib.makeBinPath [ jre ]}
     runHook postInstall
   '';

@@ -31,7 +30,6 @@ stdenv.mkDerivation rec {
     license = licenses.mit;
     description = "A build tool for Scala, Java and more";
     maintainers = with maintainers; [ scalavision ];
-    platforms = platforms.unix;
+    inherit (jre.meta) platforms;
   };
-
 }

@scalavision
Copy link
Contributor Author

Thank you very much for your feedback @veprbl :-)
I've learned a lot from this!!

@scalavision
Copy link
Contributor Author

oh.. hm... do you think there is a way to solve it? I used this as my recipe originally, I see they have used patform.all in there.

https://github.com/NixOS/nixpkgs/blob/dd46b1adb6da5e98fa5850cef6a07c29e3659fa0/pkgs/development/tools/ammonite/default.nix

@veprbl
Copy link
Member

veprbl commented Nov 29, 2018

platform.all should be fine, I think. It should raise a meaningful error if jre is not supported on the current platform.

@veprbl
Copy link
Member

veprbl commented Nov 29, 2018

@GrahamcOfBorg build mill

@GrahamcOfBorg
Copy link

No attempt on aarch64-linux (full log)

The following builds were skipped because they don't evaluate on aarch64-linux: mill

Partial log (click to expand)


a) For `nixos-rebuild` you can set
  { nixpkgs.config.allowUnfree = true; }
in configuration.nix to override this.

b) For `nix-env`, `nix-build`, `nix-shell` or any other Nix command you can add
  { allowUnfree = true; }
to ~/.config/nixpkgs/config.nix.


@GrahamcOfBorg
Copy link

Success on x86_64-darwin (full log)

Attempted: mill

Partial log (click to expand)

unpacking sources
patching sources
configuring
no configure script, doing nothing
installing
post-installation fixup
strip is /nix/store/mvpvjar6m4jpjcz48715w2pax53djv6g-cctools-binutils-darwin/bin/strip
stripping (with command strip and flags -S) in /nix/store/4qjjdb2jfqkxhvsxcnjxqmlyxgjc5jb2-mill-0.3.5/bin
patching script interpreter paths in /nix/store/4qjjdb2jfqkxhvsxcnjxqmlyxgjc5jb2-mill-0.3.5
/nix/store/4qjjdb2jfqkxhvsxcnjxqmlyxgjc5jb2-mill-0.3.5

@GrahamcOfBorg
Copy link

Success on x86_64-linux (full log)

Attempted: mill

Partial log (click to expand)

configuring
no configure script, doing nothing
installing
post-installation fixup
shrinking RPATHs of ELF executables and libraries in /nix/store/imc6ypa31lmzp1zlia8xikb82n0dkdb0-mill-0.3.5
strip is /nix/store/n4hb93w6j076xcjw5pm09rdmc09s075b-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/imc6ypa31lmzp1zlia8xikb82n0dkdb0-mill-0.3.5/bin
patching script interpreter paths in /nix/store/imc6ypa31lmzp1zlia8xikb82n0dkdb0-mill-0.3.5
checking for references to /build in /nix/store/imc6ypa31lmzp1zlia8xikb82n0dkdb0-mill-0.3.5...
/nix/store/imc6ypa31lmzp1zlia8xikb82n0dkdb0-mill-0.3.5

@veprbl
Copy link
Member

veprbl commented Nov 29, 2018

Seems to work to compile and run a simple hello world Scala project. I don't understand where it gets java and scala compiler from. @scalavision do you understand how it works?

Copy link
Member

@veprbl veprbl left a comment

Choose a reason for hiding this comment

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

Okay. I see that it does look for javac to compile java. It seems to work fine for interactive use as is. It probably will need some more work to be used inside a Nix build (e.g. some artefacts need to be fetched, probably can be done in a way similar to what's done in bloop).

I think this is good to be merged.
Just in case, cc'ing sbt maintainers @NeQuissimus @rickynils

@scalavision
Copy link
Contributor Author

I am really no sure @veprbl , it seems he has a dependency on zinc, and maybe that one does the magic. I think he has reused some stuff from sbt and zinc.

I agree, I think support for use within a nix build could be added on later. Again, thank you very much for helping out :-)

@veprbl veprbl merged commit e0e8486 into NixOS:master Dec 3, 2018
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