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.3 #91758

Merged
merged 4 commits into from Jul 28, 2020
Merged

Bloop: 1.3.4 -> 1.4.3 #91758

merged 4 commits into from Jul 28, 2020

Conversation

@Tomahna
Copy link
Contributor

Tomahna commented Jun 29, 2020

Motivation for this change

Updates bloop from 1.3.4 to 1.4.2.

During the 1.4.+ upgrade, bloop has changed the way it is packaged. While the previous way of packaging it still works, bloop now uses graalvm native images in other distributions. This PR updates the derivation in order to be as close as possible to the way bloop is packaged in the archlinux user repository

This PR should supersedes #89359, commits and attributions have been kept.

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.
@Tomahna
Copy link
Contributor Author

Tomahna commented Jun 29, 2020

@karolchmist & @svalaskevicius please take a look. The problem of non-fixed hash should be fixed.

Sorry to multiply the PRs, but it just seemed much easier to just throw my modifications into a new branch and fiddle with git than trying to update the other PR.

@karolchmist
Copy link
Contributor

karolchmist commented Jun 29, 2020

Hello @Tomahna, nice! I just tested it, works great. I have no power of merging, but it's 👍 from me.

@svalaskevicius
Copy link
Contributor

svalaskevicius commented Jun 29, 2020

Looks good to me too :) thanks!

Not too sure what's the process of actually merging it, last few times @worldofpeace helped.

@karolchmist karolchmist mentioned this pull request Jun 30, 2020
4 of 10 tasks complete
@ludovicc
Copy link

ludovicc commented Jul 3, 2020

Reviewed points
  • package name fits guidelines
  • package version fits guidelines
  • package build on MacOS Catalina
  • executables tested on MacOS Catalina
  • all depending packages build
Possible improvements
Comments
@Tomahna Tomahna force-pushed the Tomahna:bloop branch from 9827f94 to 265ad21 Jul 3, 2020
@Tomahna Tomahna changed the title Bloop: 1.3.4 -> 1.4.2 Bloop: 1.3.4 -> 1.4.3 Jul 3, 2020
@Tomahna
Copy link
Contributor Author

Tomahna commented Jul 3, 2020

Updated with bloop 1.4.3

karolchmist and others added 2 commits Jun 16, 2020
* Include all completions
* Update derivation to make it similar to archlinux packaging
@Tomahna Tomahna force-pushed the Tomahna:bloop branch from 265ad21 to e58afbc Jul 8, 2020
@worldofpeace
Copy link
Member

worldofpeace commented Jul 8, 2020

What test can I do to determine this upgrade has been successful?

@Tomahna
Copy link
Contributor Author

Tomahna commented Jul 8, 2020

You can use bloop about. It will start the build server and print you the current version (1.4.3).

Other than that, well you would have to set up a basic scala environment and build a scala project with it.

@ludovicc
Copy link

ludovicc commented Jul 9, 2020

On Darwin and with bloop 1.4.3 from your latest update, bloop about does not start the build server:

bloop about
Could not connect to server 127.0.0.1:8212

Have you forgotten to start bloop's server? Run it with `bloop server`.
Check our usage instructions in https://scalacenter.github.io/bloop/

If I install bloop using coursier, bloop about starts the build server.

@Tomahna
Copy link
Contributor Author

Tomahna commented Jul 10, 2020

It is strange, it starts the server properly on nixos.

No server running at 127.0.0.1:8212, let's fire one...
Resolving ch.epfl.scala:bloop-frontend_2.12:1.4.3...
Starting bloop server at 127.0.0.1:8212...
Attempting a connection to the server...
Attempting a connection to the server...
Attempting a connection to the server...
Attempting a connection to the server...
bloop v1.4.3

Using Scala v2.12.8 and Zinc v1.3.0-M4+45-d4354be3
Running on Java JDK v11.0.7-internal (/nix/store/q0ap8aa5br4vr2y75n7hmacwlihmpdgq-openjdk-11.0.7-ga/lib/openjdk)
  -> Supports debugging user code, Java Debug Interface (JDI) is available.
Maintained by the Scala Center (Jorge Vicente Cantero, Martin Duhem)

Can you try starting the build server manually (through the bloop server command). And send me the log of both your coursier install and the nix one.

My guess is that the installer does not select the correct binary.

@ludovicc
Copy link

ludovicc commented Jul 10, 2020

coursier install bloop --only-prebuilt=true --verbose
Using install directory /Users/lclaude/Library/Application Support/Coursier/bin
https://repo1.maven.org/maven2/io/get-coursier/apps/maven-metadata.xml
  No new update since 2020-06-30 14:51:44
Found app bloop in channel io.get-coursier:apps
https://repo1.maven.org/maven2/org/scala-lang/scala-library/maven-metadata.xml
  No new update since 2020-06-25 23:27:11
https://repo1.maven.org/maven2/ch/epfl/scala/bloopgun_2.12/maven-metadata.xml
  No new update since 2020-07-01 00:19:57
https://repo1.maven.org/maven2/ch/epfl/scala/bloopgun_2.12/maven-metadata.xml
  No new update since 2020-07-01 00:19:57
Found prebuilt launcher at https://github.com/scalacenter/bloop/releases/download/v1.4.3/bloop-x86_64-apple-darwin (37.2 MiB)
Wrote /Users/lclaude/Library/Application Support/Coursier/bin/.bloop.part
Wrote /Users/lclaude/Library/Application Support/Coursier/bin/bloop
Wrote /Users/lclaude/Library/Application Support/Coursier/bin/.bloop.aux
Wrote bloop

❯ which bloop
/Users/lclaude/Library/Application Support/Coursier/bin/bloop
❯ bloop about
No server running at 127.0.0.1:8212, let's fire one...
Resolving ch.epfl.scala:bloop-frontend_2.12:1.4.3...
Starting bloop server at 127.0.0.1:8212...
Attempting a connection to the server...
Attempting a connection to the server...
Attempting a connection to the server...
Attempting a connection to the server...
Attempting a connection to the server...
Attempting a connection to the server...
Attempting a connection to the server...
bloop v1.4.3

Using Scala v2.12.8 and Zinc v1.3.0-M4+45-d4354be3
Running on Java JDK v11.0.1 (/nix/store/sm4xb0ggmxy6sn5ahbkmrm53v8hv4lj6-zulu11.2.3-jdk11.0.1)
  -> Supports debugging user code, Java Debug Interface (JDI) is available.
Maintained by the Scala Center (Jorge Vicente Cantero, Martin Duhem)

@Tomahna
Copy link
Contributor Author

Tomahna commented Jul 11, 2020

Hi @ludovicc,

I've adapted the derivation to be able to fetch correct binaries on darwin. Unfortunately I do not have a mac to test it.

Could you give it a try ? You would have to provide the correct sha256 for darwin (line 56). I've not been able to figure out how nixos computes the recursive sha256 yet. (Or to find out how to test it for another platform).

@ludovicc
Copy link

ludovicc commented Jul 11, 2020

@Tomahna here are the logs of a build on Darwin:

Downloading https://repo1.maven.org/maven2/ch/epfl/scala/bloopgun_2.12/1.4.3/bloopgun_2.12-1.4.3.pom
Downloaded https://repo1.maven.org/maven2/ch/epfl/scala/bloopgun_2.12/1.4.3/bloopgun_2.12-1.4.3.pom
Downloading https://github.com/scalacenter/bloop/releases/download/v1.4.3/bloop-x86_64-apple-darwin
Still downloading:
https://github.com/scalacenter/bloop/releases/download/v1.4.3/bloop-x86_64-apple-darwin (73.60 %, 28741767 / 39049932)

Downloaded https://github.com/scalacenter/bloop/releases/download/v1.4.3/bloop-x86_64-apple-darwin
Wrote bloop
Warning: /nix/store/f1yca88p3swljsai9rhprhz88v7jcgbw-bloop-coursier-1.4.3 is not in your PATH
To fix that, add the following line to ~/.bashrc

export PATH="$PATH:/nix/store/f1yca88p3swljsai9rhprhz88v7jcgbw-bloop-coursier-1.4.3"
hash mismatch in fixed-output derivation '/nix/store/85sfggf1iynf2ig927gg2940bzx5zfsx-bloop-coursier-1.4.3':
  wanted: sha256:1ncl34f39mvk0zb5jl1l77cwjdg3xfnhjxbzz11pdfqw0d7wqywj
  got:    sha256:06c885w088yvh8l1r1jbrz0549gx2xvc8xr6rlxy6y27jk5655p2
note: keeping build directory '/private/var/folders/v5/83p3n3f546nc81syhhrfs9mr0000gn/T/nix-build-bloop-coursier-1.4.3.drv-0'
cannot build derivation '/nix/store/g37csdll1dw7fghlddc47c57vind95hj-bloop-1.4.3.drv': 1 dependencies couldn't be built
error: build of '/nix/store/g37csdll1dw7fghlddc47c57vind95hj-bloop-1.4.3.drv' failed

If you use 06c885w088yvh8l1r1jbrz0549gx2xvc8xr6rlxy6y27jk5655p2 as sha256 code for the darwin binary, it works well:

bloop v1.4.3

Using Scala v2.12.8 and Zinc v1.3.0-M4+45-d4354be3
Running on Java JDK v11.0.1 (/nix/store/sm4xb0ggmxy6sn5ahbkmrm53v8hv4lj6-zulu11.2.3-jdk11.0.1)
  -> Supports debugging user code, Java Debug Interface (JDI) is available.
Maintained by the Scala Center (Jorge Vicente Cantero, Martin Duhem)
@Tomahna Tomahna force-pushed the Tomahna:bloop branch from cfa8831 to 65eb6c7 Jul 11, 2020
@Tomahna
Copy link
Contributor Author

Tomahna commented Jul 11, 2020

Awesome, I've updated this PR with the provided sha.

@ludovicc
Copy link

ludovicc commented Jul 13, 2020

I re-tested on Darwin (macOs Catalina 10.15.5), it works well.

@svalaskevicius
Copy link
Contributor

svalaskevicius commented Jul 16, 2020

does anyone know what's left to be done before this can be merged? :)

@ludovicc
Copy link

ludovicc commented Jul 16, 2020

@jonringer can you review this pr? LGTM on my side.

Copy link
Contributor

jonringer left a comment

seems to only work if java on PATH:

[nix-shell:/home/jon/.cache/nixpkgs-review/pr-91758]$ nix-shell --pure -p "with import ./nixpkgs {}; bloop"
these derivations will be built:
  /nix/store/cl19rgxmxni5c2y1aw8bjnwq3ick49b9-bloop-1.4.3.drv
...

[nix-shell:/home/jon/.cache/nixpkgs-review/pr-91758]$ bloop --help
No server running at 127.0.0.1:8212, let's fire one...
Resolving ch.epfl.scala:bloop-frontend_2.12:1.4.3...
Starting bloop server at 127.0.0.1:8212...
java.io.IOException: Cannot run program "java" (in directory "/home/jon/.cache/nixpkgs-review/pr-91758"
....

[nix-shell:/home/jon/.cache/nixpkgs-review/pr-91758]$ nix-shell --pure -p "with import ./nixpkgs {}; [ bloop openjdk.jre ]"

[nix-shell:/home/jon/.cache/nixpkgs-review/pr-91758]$ bloop about
No server running at 127.0.0.1:8212, let's fire one...
Resolving ch.epfl.scala:bloop-frontend_2.12:1.4.3...
Starting bloop server at 127.0.0.1:8212...
Attempting a connection to the server...
Attempting a connection to the server...
Attempting a connection to the server...
bloop v1.4.3

Using Scala v2.12.8 and Zinc v1.3.0-M4+45-d4354be3
Running on Java JRE v1.8.0_242 (/nix/store/7qknskbanpwq7a71s2n6dv3l5b3frj7d-openjdk-8u242-b08-jre/lib/openjdk/jre)
  -> Doesn't support debugging user code, runtime doesn't implement Java Debug Interface (JDI).
Maintained by the Scala Center (Jorge Vicente Cantero, Martin Duhem)
@Tomahna
Copy link
Contributor Author

Tomahna commented Jul 16, 2020

I'm not sure what to do about that. The precompiled binary is expecting Java on the classpath, I don't see how we can patch that.

Should the derivation do something about it ? Is it really a problem ?

@jonringer
Copy link
Contributor

jonringer commented Jul 16, 2020

wrapping doesn't seem to work, but doing propagatedBuildInputs = [ openjdk.jre ]; does work


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

This comment has been minimized.

Copy link
@jonringer

jonringer Jul 16, 2020

Contributor

fix java missing issue

Suggested change
buildInputs = [ stdenv.cc.cc.lib zlib ] ;
buildInputs = [ stdenv.cc.cc.lib zlib ] ;
propagatedBuildInputs = [ openjdk.jre ];

This comment has been minimized.

Copy link
@jonringer

jonringer Jul 16, 2020

Contributor

this does increase the closure size quite a bit:

[nix-shell:/home/jon/.cache/nixpkgs-review/pr-91758-4]$ nix path-info -Sh ./results/bloop
/nix/store/fp1kc7lxw0rhhs6my624wm154hn8494w-bloop-1.4.3	  77.2M

[nix-shell:/home/jon/.cache/nixpkgs-review/pr-91758-4]$ nix path-info -Sh ./result
/nix/store/c0ls7dh4vnjd4wyqib3mz6yyfz2n3g49-bloop-1.4.3	 434.8M

However, it's more representative of it's actual runtime closure:

[16:52:28] jon@jon-desktop /home/jon/projects/nixpkgs (buildDotnetCorePackage)
$ nix path-info -Sh ./result-jre
/nix/store/7qknskbanpwq7a71s2n6dv3l5b3frj7d-openjdk-8u242-b08-jre	 396.5M

This comment has been minimized.

Copy link
@svalaskevicius

svalaskevicius Jul 17, 2020

Contributor

This does still mean that we can change to any java we want when using bloop, right? :)

In fact, why are we adding java 8? Does the build not work without it or usage?

This comment has been minimized.

Copy link
@svalaskevicius

svalaskevicius Jul 17, 2020

Contributor

Looks like a similar thing is done for sbt already - https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/tools/build-managers/sbt/default.nix, and we can override jvm for bloop processes, so yeah (answering my own question) :)

This comment has been minimized.

Copy link
@svalaskevicius

svalaskevicius Jul 17, 2020

Contributor

Would be good if there was a way to configure preferred java provider (or at least the major version) in one place though, but thats much more generic question than this PR

This comment has been minimized.

Copy link
@jonringer

jonringer Jul 17, 2020

Contributor

as with most things with nix, there's a way to do anything, it's just how much pain is it to introduce flexibility. I guess you could pass jre to this derivation, and then just say:

  bloop = callPackage ../.. {
    jre = openjdkXX.jre;
  };

and then if you want to change it, then you would do something like:

  bloop.override { jre = blah.jre; };

This comment has been minimized.

Copy link
@Tomahna

Tomahna Jul 18, 2020

Author Contributor

It does not seem necessary to explicitly pass the jre in the callPackage call, several other derivations which use jre do not do it (coursier, scalafmt, metals, ...).

However by passing the jre inside the derivation, it seems to respect the programs.java.package property or can be overriden with derivation.override.

};

nativeBuildInputs = [ autoPatchelfHook ] ;
phases = [ "installPhase" "fixupPhase" ];

This comment has been minimized.

Copy link
@worldofpeace

worldofpeace Jul 17, 2020

Member

manually defining phases is deprecated. See the stdenv chapter of the nixpkgs manual to disable phases like dontBuild = true; etc.

This comment has been minimized.

Copy link
@Tomahna

Tomahna Jul 18, 2020

Author Contributor

It's replaced by dontUnpack = true.

else throw "unsupported platform";
};

nativeBuildInputs = [ autoPatchelfHook ] ;

This comment has been minimized.

Copy link
@worldofpeace

worldofpeace Jul 17, 2020

Member
Suggested change
nativeBuildInputs = [ autoPatchelfHook ] ;
nativeBuildInputs = [ autoPatchelfHook ];

this is done in various places

This comment has been minimized.

Copy link
@Tomahna

Tomahna Jul 18, 2020

Author Contributor

Should be fixed

@Tomahna Tomahna force-pushed the Tomahna:bloop branch from 65eb6c7 to a6715ae Jul 18, 2020
@Tomahna
Copy link
Contributor Author

Tomahna commented Jul 18, 2020

All your review comments should have been taken into account and fixed.

@svalaskevicius
Copy link
Contributor

svalaskevicius commented Jul 28, 2020

@worldofpeace @jonringer hi, please can you review if you're happy with the changes?

Would be good to get this merged as the old bloop version is not really usable anymore (not compatible with the new scala version etc)

Copy link
Contributor

jonringer left a comment

LGTM

Result of nixpkgs-review pr 91758 1

1 package built: - bloop

@jonringer jonringer merged commit df6e489 into NixOS:master Jul 28, 2020
14 checks passed
14 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="a6715ae"; rev="a6715ae510b308320c42e80fb351d0de4a2b34c0"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="a6715ae"; rev="a6715ae510b308320c42e80fb351d0de4a2b34c0"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="a6715ae"; rev="a6715ae510b308320c42e80fb351d0de4a2b34c0"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="a6715ae"; rev="a6715ae510b308320c42e80fb351d0de4a2b34c0"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="a6715ae"; rev="a6715ae510b308320c42e80fb351d0de4a2b34c0"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="a6715ae"; rev="a6715ae510b308320c42e80fb351d0de4a2b34c0"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="a6715ae"; rev="a6715ae510b308320c42e80fb351d0de4a2b34c0"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@jonringer
Copy link
Contributor

jonringer commented Jul 28, 2020

sorry, went on vacation for a week and a half

@svalaskevicius
Copy link
Contributor

svalaskevicius commented Jul 28, 2020

thanks!

also thanks to everyone involved, especially @Tomahna - for continuously maintaining this derivation - over a few bloop releases now :)

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

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