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

clojupyter: init at 0.3.2 #103560

Merged
merged 1 commit into from Aug 20, 2021
Merged

clojupyter: init at 0.3.2 #103560

merged 1 commit into from Aug 20, 2021

Conversation

thomasjm
Copy link
Contributor

Motivation for this change

This PR attempts to address #72464 by packaging clojupyter.

It almost works, but there is some kind of Java issue making the final JAR file unable to run. It seems to be a classpath issue, and it doesn't happen when building outside of Nix. When you try to run the JAR file inside the buildPhase right after building it, you get java.lang.NoClassDefFoundError: io/simplect/compose$?. As far as I can tell, it's trying to load a class that doesn't exist (and has a name that's a single question mark?). In a weird way this is a bit like issue #75011 due to the extraneous question mark.

I'm hoping someone familiar with Clojure in Nix can help push this over the finish line. A few pings:

Why does this use a fixed-output derivation?

I tried to find a more Nix-y way to do this, but building Clojure applications in Nix is a little tricky at the moment as discussed here: https://discourse.nixos.org/t/maintained-usable-tooling-for-building-clojure-projects-in-nix/1556/9.

  • I tried using mvn2nix, but hit some blockers and filed an issue there
  • I tried using clj2nix, but had trouble figuring out how to use it with Leiningen
  • I think the FOD should be okay, because the dependencies are nailed down in deps.edn and the lein build (combined with the use of strip-nondeterminism) should therefore be reproducible. However in practice I'm seeing it get different hashes on each build, which raises another question about whether lein uberjar can be made reproducible.
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 (Ubuntu)
  • [N/A] Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • [N/A] 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/)
  • [N/A] Determined the impact on package closure size (by running nix path-info -S before and after)
  • [N/A] Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

cp "$uberjar" $out/share/clojupyter
'';

# doCheck = false;
Copy link
Member

Choose a reason for hiding this comment

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

Do the tests work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure! I haven't tried because we need to prove we can get past the classpath and reproducibility issues first.

@thomasjm
Copy link
Contributor Author

Thanks for the review @SuperSandro2000 !

@stale
Copy link

stale bot commented Jul 20, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 20, 2021
@bendlas
Copy link
Contributor

bendlas commented Jul 21, 2021

Sorry to be late to the party.

I've rebased this and taken it for a spin: https://github.com/bendlas/nixpkgs/tree/clojupyter

First thing I noticed was that the build seemed to access my user's .m2 repository, but that was a red herring, and actually a duplicate of NixOS/nix#3693. Thanks @dasJ for pointing me to this.

So after applying the LD_PRELOAD hack to disable nscd, I got the build to run, but had to update the fixed-output hash.

I then changed the build to unpack the standalone jar into the target directory. The jvm is just as happy to run bytecode from a directory of .class files, as it is running from a jar. I suspect that this resolves the repro issue (please verify by rebuilding my branch), because the .jar format isn't exactly geared towards reproducability. IIRC, .nar files are carefully geared for that use case.

In addition to verifying this fix, I'd also like to discuss the options for getting rid of fixed output:

dwn: thanks for bringing it up. the way i conceptualized it, it's very much intended to solve packaging like this. unfortunately, it has gotten too unfocused to be useful, and it's not very modular. I'm addressing these issues by building it up straight on lib/modules.nix. First attempts look promising, but it may take any amount of time before I can recommend either the current or reworked approach. OTOH, some of it's functionality around resolving dependencies into class paths won't need to change very much any more. The reason I haven't pushed for upstreaming them was, that there is already mvn2nix, and before proposing to reconcile with or replace it, I'd like dwn's story to be fully straight. But if people are interested in this, I'd be happy to collaborate.

mvn2nix: do you have a link to the issue, you mentioned, @thomasjm? as far as I'm aware, it should work fine together with https://github.com/talios/clojure-maven-plugin and its main problem is just the sheer bloat of its generated files (due to internal duplication)

clj2nix: cool, I hadn't seen that. In theory, it should be pretty easy to just convert (a subset of) a project.clj to a deps.edn, but why bother? You could just run it on a custom deps.edn, that has a dependency on https://clojars.org/clojupyter

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 21, 2021
@thomasjm
Copy link
Contributor Author

Thanks @bendlas , I'll try your branch now.

This looks like the issue I mentioned: fzakaria/mvn2nix#30. It doesn't look like a major blocker though so I'm not sure what I was talking about, maybe I just didn't get very far with mvn2nix.

@bendlas
Copy link
Contributor

bendlas commented Jul 21, 2021

going with clj2nix seems most painless, for now bendlas@62fa08e

@thomasjm
Copy link
Contributor Author

@bendlas just tried 62fa08e. I had to add git to buildInputs to avoid an error about missing git.

After that, it finished building successfully but then I got a hash mismatch for the FOD. Jar file nondeterminism I guess?

@bendlas
Copy link
Contributor

bendlas commented Jul 22, 2021

@bendlas just tried 62fa08e. I had to add git to buildInputs to avoid an error about missing git.

oops, sorry, the fixed hash masked this ..

After that, it finished building successfully but then I got a hash mismatch for the FOD. Jar file nondeterminism I guess?

That's strange, because if you look into result, there shouldn't be a jar file any more, but unpacked .class files.

In any case, the commit you mentioned also has ./update.sh, which should generate deps.nix as per clj2nix.

That would have the advantage of sharing artifact downloads with mvn2nix, and in fact having any download caching at all.

@thomasjm
Copy link
Contributor Author

That's strange, because if you look into result, there shouldn't be a jar file any more, but unpacked .class files.

You're right, I was just guessing. It is unpacked .class files.

I ran it on two machines and got seemingly nondeterministic output. There are about 20k files in the result and differences in 195 of them (diff output: diff.log).

I'll try playing with the deps.nix next...

@thomasjm
Copy link
Contributor Author

I think the deps.nix approach is almost working, I just can't quite figure out how to launch the kernel. Something like this:

{ pkgs, jre, writeShellScript }:

let cljdeps = import ./deps.nix { inherit pkgs; };
    classp  = cljdeps.makeClasspaths {};

in

writeShellScript "clojupyter" ''
  ${jre}/bin/java -cp ${classp} clojupyter.kernel.core $*
''

@bendlas
Copy link
Contributor

bendlas commented Jul 23, 2021

if i'm reading this correctly, it looks like you need to pass a connection file argument, and that you do that from the kernel descriptor json via placeholder: https://github.com/clojupyter/clojupyter/blob/b54b30b5efa115937b7a85e708a7402bd9efa0ab/src/clojupyter/util.clj#L138

does the nixos jupyter package support generating kernel.json?

@thomasjm
Copy link
Contributor Author

Yes, I'll put a kernel definition in this PR for the final version. I was just slightly concerned because I was running the script with random arguments and it always just exited silently. I'll try it with a real connection file though.

@thomasjm
Copy link
Contributor Author

Okay, I think it's working! I added a snippet in the main file showing how to launch Jupyter Notebook with it.

Many thanks @bendlas. @SuperSandro2000 could you take another look?

# $ nix-prefetch-github hlolli clj2nix

nix-shell --run "clj2nix deps.edn deps.nix" -E '
with import <nixpkgs> { };
Copy link
Member

Choose a reason for hiding this comment

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

Use a relative path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I don't quite understand (this code was kindly provided by @bendlas) -- relative path to what?

Copy link
Contributor

@bendlas bendlas Jul 23, 2021

Choose a reason for hiding this comment

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

I think the idea is smth like ../../../../default.nix, instead of <nixpkgs>. The reason I left <nixpkgs> was, that the script could be called from another working dir, and I didn't want to mess with bash-isms to get the script directory.

EDIT Actually scratch that first point, the script has a dependency on its WD anyway, for finding deps.edn and deps.nix.

Also, I thought building the updater from whatever <nixpkgs> is current, may even be more appropriate, than building the current system to build the updater in this case. The intended audience for this script are only devs, hacking on nixpkgs.

OTOH, I only hacked this up as a proposal. No qualms about having it another way. I was actually thinking, that clj2nix should probably be packaged with nixpkgs, if we were to do this approach.

Copy link
Member

Choose a reason for hiding this comment

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

It should use the current checkout because it is not guaranteed that the nixpkgs channel exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just changed it to a relative path and confirmed it works, although I tend to agree with @bendlas that it makes more sense to use <nixpkgs> since this update script is only for maintainers, and this relative path makes it more fragile to moving the file around etc.

Either way, this PR is ready for another look!

Copy link
Member

Choose a reason for hiding this comment

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

this relative path makes it more fragile to moving the file around etc.

The file usually does not move around and when you use the nix-channel you can easily bypass local changes and wonder why they are not used in the updater.

@SuperSandro2000
Copy link
Member

Please clenaup the git history and squash all the package related commits.

@thomasjm
Copy link
Contributor Author

Aah sorry for the reviewer noise, squashed and rebased on master now.

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

3 participants