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

Bloop: 1.3.4 -> 1.4.1 #89359

Closed
wants to merge 2 commits into from
Closed

Bloop: 1.3.4 -> 1.4.1 #89359

wants to merge 2 commits into from

Conversation

@karolchmist
Copy link
Contributor

karolchmist commented Jun 2, 2020

Motivation for this change

Update bloop to the latest version

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • 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 nixpkgs-review --run "nixpkgs-review wip"
  • 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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

cc maintainer @Tomahna

@karolchmist
Copy link
Contributor Author

karolchmist commented Jun 2, 2020

There is one thing I'm not sure about.

For the cli, coursier install creates two files: bloop and .bloop.aux.

I link ${client}/.bloop.aux to $out/bin/bloop as I couldn't find a way to make it work otherwise.

I tried to create a link to both bloop and .bloop.aux, so the output would be:

/nix/store/ipbc12sqrg3abyxyyvf6rf3a2dmvxx7w-bloop-1.4.1/bin:
lrwxrwxrwx 1 root root   68 Jan  1  1970 bloop -> /nix/store/w2crq019aq7hz4wgp8h4lbfp6gjpam74-bloop-client-1.4.1/bloop
lrwxrwxrwx 1 root root   73 Jan  1  1970 .bloop.aux -> /nix/store/w2crq019aq7hz4wgp8h4lbfp6gjpam74-bloop-client-1.4.1/.bloop.aux
lrwxrwxrwx 1 root root   73 Jan  1  1970 blp-server -> /nix/store/zg1nzydhffdzwm7yi97kl86c613rhdjb-bloop-server-1.4.1/blp-server

but it doesn't work :

/home/karol/.nix-profile/bin/bloop: line 4: /home/karol/.nix-profile/bin/.bloop.aux: No such file or directory

First few lines of bloop is this:

#!/nix/store/81wybawvkr95c7j8gj5ab3y740mq1fli-bash-4.4-p23/bin/sh
export CS_NATIVE_LAUNCHER="true"
export IS_CS_INSTALLED_LAUNCHER="true"
exec "$(cd "$(dirname "$0")"; pwd)/.bloop.aux" "$@"
[SHORT BINARY]

So it's a wrapper with two additional variables set and some binary code (not sure what it does).

It seems that nixpkgs will link only visible files from $out/bin to .nix-profile, so .bloop.aux is ignored:

ll /home/karol/.nix-profile/bin | grep bloop

lrwxrwxrwx 1 root root    65 Jan  1  1970 bloop -> /nix/store/ipbc12sqrg3abyxyyvf6rf3a2dmvxx7w-bloop-1.4.1/bin/bloop*
lrwxrwxrwx 1 root root    70 Jan  1  1970 blp-server -> /nix/store/ipbc12sqrg3abyxyyvf6rf3a2dmvxx7w-bloop-1.4.1/bin/blp-server*

It works in the end, but maybe there is a better solution....

This may be relevant, how the Arch package is produced

@fabianpage
Copy link
Contributor

fabianpage commented Jun 14, 2020

are there any updates on this mr?

@Tomahna
Copy link
Contributor

Tomahna commented Jun 14, 2020

So first of all, sorry of letting this wait for so long. I've been aiming to review this for more than a week now.

Note that the old way of packaging still work with bloop 1.4.+.

Currently, I'm not able to build this. I end up with an exception. (looking into it)

Downloaded 56 missing file(s) / 56
Exception in thread "main" java.nio.file.AccessDeniedException: /home
        at java.base/sun.nio.fs.UnixException.translateToIOException(UnixException.java:90)
        at java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:111)
        at java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:116)
        at java.base/sun.nio.fs.UnixFileSystemProvider.createDirectory(UnixFileSystemProvider.java:389)
        at java.base/java.nio.file.Files.createDirectory(Files.java:689)
        at java.base/java.nio.file.Files.createAndCheckIsDirectory(Files.java:796)
        at java.base/java.nio.file.Files.createDirectories(Files.java:782)
        at coursier.paths.Util.createDirectories(Util.java:80)
        at coursier.paths.CoursierPaths.computeCacheDirectory(CoursierPaths.java:68)
        at coursier.paths.CoursierPaths.computeJvmCacheDirectory(CoursierPaths.java:41)
        at coursier.paths.CoursierPaths.jvmCacheDirectory(CoursierPaths.java:91)
        at coursier.jvm.JvmCache$.defaultBaseDirectory0$lzycompute(JvmCache.scala:372)
        at coursier.jvm.JvmCache$.defaultBaseDirectory0(JvmCache.scala:371)
        at coursier.jvm.JvmCache$.defaultBaseDirectory(JvmCache.scala:308)
        at coursier.cli.jvm.SharedJavaParams$.$anonfun$apply$5(SharedJavaParams.scala:60)
        at scala.Option.getOrElse(Option.scala:189)
        at coursier.cli.jvm.SharedJavaParams$.apply(SharedJavaParams.scala:60)
        at coursier.cli.install.InstallParams$.apply(InstallParams.scala:38)
        at coursier.cli.install.Install$.run(Install.scala:20)
        at coursier.cli.Coursier$.$anonfun$runA$2(Coursier.scala:121)
        at coursier.cli.Coursier$.$anonfun$runA$2$adapted(Coursier.scala:113)
        at coursier.cli.CommandAppPreA.run(CommandAppPreA.scala:22)
        at caseapp.core.app.CommandAppWithPreCommand.$anonfun$main$5(CommandAppWithPreCommand.scala:99)
        at caseapp.core.app.CommandAppWithPreCommand.$anonfun$main$5$adapted(CommandAppWithPreCommand.scala:99)
        at scala.util.Either.fold(Either.scala:191)
        at caseapp.core.app.CommandAppWithPreCommand.$anonfun$main$3(CommandAppWithPreCommand.scala:99)
        at caseapp.core.app.CommandAppWithPreCommand.$anonfun$main$3$adapted(CommandAppWithPreCommand.scala:85)
        at scala.Option.foreach(Option.scala:407)
        at caseapp.core.app.CommandAppWithPreCommand.main(CommandAppWithPreCommand.scala:85)
        at coursier.cli.Coursier$.main(Coursier.scala:78)
        at coursier.cli.Coursier.main(Coursier.scala)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.base/java.lang.reflect.Method.invoke(Method.java:566)
        at coursier.bootstrap.launcher.a.a(Unknown Source)
        at coursier.bootstrap.launcher.Launcher.main(Unknown Source)
builder for '/nix/store/8as2l4znw6zi7gpk9x06637i8998nwm1-bloop-client-1.4.1.drv' failed with exit code 1```
@Tomahna
Copy link
Contributor

Tomahna commented Jun 14, 2020

Ok so for the problem of AccessDeniedException: /home, adding export COURSIER_JVM_CACHE=$(pwd) whenever using coursier install is necessary.

I link ${client}/.bloop.aux to $out/bin/bloop as I couldn't find a way to make it work otherwise.

The bloop script use a relative path to find .bloop.aux, it does not follow relative link. The AUR package faces the same problem and replace $(dirname "$0") by the effective path. Something along the line of sed 's|$(dirname "$0")|$out|' -i $out/bloop would do the trick if escaped correctly.

Along the line of following more closely what is done in the AUR package, I have got something along this line. (Note that it does not work yet, it's still a work in progress)

{ stdenv, fetchurl, coursier,
 autoPatchelfHook, 
 lib, zlib }:

stdenv.mkDerivation rec {
  version = "1.4.1";
  name = "bloop-${version}";

  bloop-coursier-channel = fetchurl {
    url = "https://github.com/scalacenter/bloop/releases/download/v${version}/bloop-coursier.json";
    sha256 = "2e6a873183e5e22712913bfdab1331d0a7792167c369fee5be0c83e27572fe12";
  };

  bloop-bash = fetchurl {
    url = "https://github.com/scalacenter/bloop/releases/download/v${version}/bash-completions";
    sha256 = "da6b7ecd4109bd0ff98b1c452d9dd9d26eee0d28ff604f6c83fb8d3236a6bdd1";
  };

  bloop-fish = fetchurl {
    url = "https://github.com/scalacenter/bloop/releases/download/v${version}/fish-completions";
    sha256 = "1pa8h81l2498q8dbd83fzipr99myjwxpy8xdgzhvqzdmfv6aa4m0";
  };

  bloop-zsh = fetchurl {
    url = "https://github.com/scalacenter/bloop/releases/download/v${version}/zsh-completions";
    sha256 = "1xzg0qfkjdmzm3mvg82mc4iia8cl7b6vbl8ng4ir2xsz00zjrlsq";
  };

  buildCommand = ''
    export COURSIER_CACHE=$(pwd)
    export COURSIER_JVM_CACHE=$(pwd)

    mkdir -p channel
    ln -s ${bloop-coursier-channel} channel/bloop.json
    ${coursier}/bin/coursier install --install-dir $out --default-channels=false --channel channel --only-prebuilt=true bloop

    # patch the bloop launcher so that it works when symlinked
    sed 's|$(dirname "$0")|$out|' -i $out/bloop

    mkdir -p $out/bin
    ln -s $out/bloop $out/bin/bloop

    patchelf --set-interpreter $(cat $NIX_CC/nix-support/dynamic-linker) $out/.bloop.aux

    #Install completions
    mkdir -p $out/etc/bash_completion.d
    ln -s ${bloop-bash} $out/etc/bash_completion.d/bloop
    mkdir -p $out/share/zsh/site-functions
    ln -s ${bloop-zsh} $out/share/zsh/site-functions/_bloop
    mkdir -p $out/usr/share/fish/vendor_completions.d/
    ln -s ${bloop-fish} $out/usr/share/fish/vendor_completions.d/bloop.fish
  '';

  outputHashMode = "recursive";
  outputHashAlgo = "sha256";
  outputHash     = "1z12cg44z56jgsv754m1ig4xifawqlxs898r95lqi39zsb7lshgi";

  meta = with stdenv.lib; {
    homepage = "https://scalacenter.github.io/bloop/";
    license = licenses.asl20;
    description = "Bloop is a Scala build server and command-line tool to make the compile and test developer workflows fast and productive in a build-tool-agnostic way.";
    maintainers = with maintainers; [ tomahna ];
  };
}

The original proposal needs at least the export COURSIER_JVM_CACHE=$(pwd) part to work properly. I'll try to look further into it in the next few days.

@Tomahna
Copy link
Contributor

Tomahna commented Jun 15, 2020

{ stdenv, fetchurl, coursier,
 autoPatchelfHook, 
 lib, zlib }:

stdenv.mkDerivation rec {
  basename = "bloop";
  version = "1.4.1";
  name = "bloop-${version}";

  bloop-coursier-channel = fetchurl {
    url = "https://github.com/scalacenter/bloop/releases/download/v${version}/bloop-coursier.json";
    sha256 = "2e6a873183e5e22712913bfdab1331d0a7792167c369fee5be0c83e27572fe12";
  };

  bloop-bash = fetchurl {
    url = "https://github.com/scalacenter/bloop/releases/download/v${version}/bash-completions";
    sha256 = "da6b7ecd4109bd0ff98b1c452d9dd9d26eee0d28ff604f6c83fb8d3236a6bdd1";
  };

  bloop-fish = fetchurl {
    url = "https://github.com/scalacenter/bloop/releases/download/v${version}/fish-completions";
    sha256 = "1pa8h81l2498q8dbd83fzipr99myjwxpy8xdgzhvqzdmfv6aa4m0";
  };

  bloop-zsh = fetchurl {
    url = "https://github.com/scalacenter/bloop/releases/download/v${version}/zsh-completions";
    sha256 = "1xzg0qfkjdmzm3mvg82mc4iia8cl7b6vbl8ng4ir2xsz00zjrlsq";
  };

  bloop-coursier = stdenv.mkDerivation {
    name = "${basename}-coursier-${version}";

    nativeBuildInputs = [ autoPatchelfHook ] ;
    phases = [ "installPhase" "fixupPhase" ];
    buildInputs = [ stdenv.cc.cc.lib zlib ] ;

    installPhase = ''
      export COURSIER_CACHE=$(pwd)
      export COURSIER_JVM_CACHE=$(pwd)

      mkdir channel
      ln -s ${bloop-coursier-channel} channel/bloop.json
      ${coursier}/bin/coursier install --install-dir $out --default-channels=false --channel channel --only-prebuilt=true bloop

      # Remove binary part of the coursier launcher script to make derivation output hash stable
      sed -i '5,$ d' $out/bloop
   '';

    outputHashMode = "recursive";
    outputHashAlgo = "sha256";
    outputHash     = "0x74h420yjjyvjk0nhpghp9y9vi9l1dxpqq307wpv9fms1a2ascj";
  };

  buildCommand = ''
    export COURSIER_CACHE=$(pwd)
    export COURSIER_JVM_CACHE=$(pwd)

    mkdir -p $out/bin
    cp ${bloop-coursier}/bloop $out/bloop
    ln -s ${bloop-coursier}/.bloop.aux $out/.bloop.aux
    ln -s $out/bloop $out/bin/bloop

    # patch the bloop launcher so that it works when symlinked
    sed "s|\$(dirname \"\$0\")|$out|" -i $out/bloop

    #Install completions
    mkdir -p $out/etc/bash_completion.d
    ln -s ${bloop-bash} $out/etc/bash_completion.d/bloop
    mkdir -p $out/share/zsh/site-functions
    ln -s ${bloop-zsh} $out/share/zsh/site-functions/_bloop
    mkdir -p $out/usr/share/fish/vendor_completions.d/
    ln -s ${bloop-fish} $out/usr/share/fish/vendor_completions.d/bloop.fish
  '';

  meta = with stdenv.lib; {
    homepage = "https://scalacenter.github.io/bloop/";
    license = licenses.asl20;
    description = "Bloop is a Scala build server and command-line tool to make the compile and test developer workflows fast and productive in a build-tool-agnostic way.";
    maintainers = with maintainers; [ tomahna ];
  };
}

Ok the above seems to works properly.

A few small gotcha's:

  • It seems that fixed output derivation (outputHash = ...) do not like when you reference their own path inside their files. That's why I'm forced to use the bloop-coursier subderivation and patch the bloop script afterwards.
  • The coursier install command create a launcher script which change sha256 at every subsequent run. The binary part of the script must include a date. That's why it is removed. It would probably be nice to be able to add the option in coursier to have an idempotent script generation.

How do you prefer to proceed with this MR @karolchmist ? I see 3 way of going forward (if you see any other, feel free to suggest):

  1. You update the PR as is with COURSIER_JVM_CACHE, I'll recheck that it works and open a subsequent PR with my changes
  2. You adapt the PR with these changes, check that it works properly for you too.
  3. I can open a new PR that supersedes this one.

I'm fine with all options. My preference is with 2 as it limits the number of PR (contrary to 1) and don't make disappear the work you've put into this (contrary to 3).

@karolchmist karolchmist force-pushed the karolchmist:bloop-1.4.1 branch from e36fc18 to e7dc75e Jun 17, 2020
@karolchmist
Copy link
Contributor Author

karolchmist commented Jun 17, 2020

Hello @Tomahna, thank you for your version, it's much nicer now 👍

Option 2 seems the best to me as well, so I committed your last suggestion (with modified outputHash, as you said it changes every build).

I rebased on latest master.

For me it's good to merge, if you agree.

@Tomahna
Copy link
Contributor

Tomahna commented Jun 17, 2020

We have a misunderstanding. The outputHash should not change, it actually has to be constant for the derivation to work otherwise other people will not be able to use it properly. I thought that removing the binary part of the coursier script would be sufficient but it appears it is not.

The problem seems to be that we cannot patch the binary and the shell script in a fixed output derivation. This makes sense as by patching the binary and the script, we make their output hash dependent of the environment (the version of bash, the version of zlib, etc). And each time the environment change, the outputHash will change. It is something we need to avoid.

We can apply the patches in the final derivation, but I guess it would mean we need to copy (instead of just link it) the bloop binary in the final directory. So we would have the binary twice in the nix-store. I don't know if someone more proficient with nix than me sees another way out of it ?

@svalaskevicius svalaskevicius mentioned this pull request Jun 26, 2020
3 of 10 tasks complete
@svalaskevicius
Copy link
Contributor

svalaskevicius commented Jun 26, 2020

if of any value, I had an alternative implementation (running it for a while, just made the pr now) : https://github.com/NixOS/nixpkgs/pull/91635/files (closed the pr as found this one :) )

@svalaskevicius
Copy link
Contributor

svalaskevicius commented Jun 26, 2020

anw, the PR here seems good (though haven't tried) - how can we get it merged? :)

update: re-read the comment above, I haven't noticed the output hash to change in my version but I'll do more checking :)

ln -s ${server}/bin/blp-server $out/blp-server
ln -s ${zsh} $out/share/zsh/site-functions/_bloop
# patch the bloop launcher so that it works when symlinked
sed "s|\$(dirname \"\$0\")|$out|" -i $out/bloop

This comment has been minimized.

Copy link
@svalaskevicius

svalaskevicius Jun 26, 2020

Contributor

nitpicking/optional: this keeps the $(cd "$out" ; pwd) part, and could be replaced wider I'd think... in my version I had #\$(cd "\$(dirname "\$0")"; pwd)/.bloop.aux#'"$out"'/bin/.bloop.aux#

@svalaskevicius
Copy link
Contributor

svalaskevicius commented Jun 26, 2020

We can apply the patches in the final derivation, but I guess it would mean we need to copy (instead of just link it) the bloop binary in the final directory. So we would have the binary twice in the nix-store. I don't know if someone more proficient with nix than me sees another way out of it ?

I don't see a better approach than this suggestion (yes it's 40MB less on my disk but hey, when I run nix-gc I get 100GB back!), in fact, the final patching I'd say is not build, but install phase, isn't it?

Tomahna added a commit to Tomahna/nixpkgs that referenced this pull request Jun 29, 2020
@Tomahna Tomahna mentioned this pull request Jun 29, 2020
3 of 10 tasks complete
@karolchmist
Copy link
Contributor Author

karolchmist commented Jun 30, 2020

Superseded by #91758, closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.