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.

Sign up for GitHub

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

Jump to bottom

jruby: changes to make it possible to use in development environment #27273

Merged
merged 3 commits into from
Jul 19, 2017
Merged

jruby: changes to make it possible to use in development environment #27273

merged 3 commits into from
Jul 19, 2017
+35 −6

Conversation

asppsa
Copy link
Contributor

@asppsa asppsa commented Jul 10, 2017 •
edited
Loading

Motivation for this change

When attempting to run JRuby (in Linux), I encountered a number of issues:

  • running gem errored out with syntax error, unexpected tSTRING_BEG;
  • it was not possible to use the jruby package with the bundler and bundix packages, due to errors to do with missing attributes in the jruby derivation, and missing environment variables.

This pull request does the following:

  • Updates jruby to the latest version;
  • Removes unnecessary JAVA_HOME wrapping from some jruby executables, such as gem;
  • Adds the rubyEngine and gemPath attributes, required by gem packages (e.g. bundler);
  • Copies some GEM_PATH envvar magic from the ruby package;
  • Adds a ruby -> jruby symlink, so that /usr/bin/env ruby hashbangs get resolved correctly;
  • Fixes the bundix package to inherit its ruby runtime from bundler (otherwise bundler could be built with one ruby, and bundix with another);

These changes make it possible for me to create a development environment for jruby as follows:

myJRubyEnv = callPackage <nixpkgs/pkgs/development/interpreters/ruby/dev.nix> {
  ruby = jruby;
};

I haven't been able to run nox-review due to some errors to do with golang and GHC. I am presuming that these are not errors caused by my patch.

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • 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"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

Sorry, something went wrong.

0xABAB
0xABAB approved these changes Jul 10, 2017
View reviewed changes
Copy link
Contributor

@0xABAB 0xABAB left a comment

Choose a reason for hiding this comment

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

In this case your changes seem trivial, but the consensus is that we want version bumps in separate PRs.

@asppsa
Copy link
Contributor Author

asppsa commented Jul 10, 2017

Ok, I'll follow that practice in the future. Sorry about that.

@Mic92 Mic92 requested a review from zimbatm July 10, 2017 10:22
zimbatm
zimbatm reviewed Jul 11, 2017
View reviewed changes
Copy link
Member

@zimbatm zimbatm left a comment

Choose a reason for hiding this comment

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

👍 for the direction

pkgs/development/interpreters/jruby/default.nix Outdated
'';

rubyEngine = "jruby";
gemPath = "lib/${rubyEngine}/gems/${version}";
Copy link
Member

Choose a reason for hiding this comment

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

These need to be mover to the passthru as they are not used by the build but as attributes when passing the derivation around.

Copy link
Member

Choose a reason for hiding this comment

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

It might also be a good idea to add the libPath

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 don't really understand passthru very well, but my understanding was that variables go in there so that they can be changed without requiring a rebuild. However, the gemPath (which depends on rubyEngine) is used in the installPhase, so it doesn't seem to fit that requirement. Happy to make this change though if you think it best.

asppsa added 3 commits July 19, 2017 16:13
@asppsa
jruby: only wrap jruby executables
5c6e4ea 
Other executables all invoke jruby, and so don't need to be wrapped.

In some cases wrapping breaks the executables because one file is a ruby
script that directly loads the other (e.g. `gem` is a wrapper that loads
`jgem`).  In this case, if the latter script has been wrapped by nix,
loading will fail.
@asppsa
bundix: use same ruby as bundler
6893589
@asppsa
jruby: make package compatible with bundix; add devEnv
0383c0a 
- Adds the necessary passthru vars for bundler, bundix, etc. to accept
  the package as a ruby.
- Adds the devEnv attribute, so that jruby.devEnv can be used to get
  an environment with bundler and bundix installed.
@asppsa
Copy link
Contributor Author

asppsa commented Jul 19, 2017 •
edited
Loading

@zimbatm, I've just pushed up a revised set of changes. Differences are:

  • version bump no longer present, as it has already happened via another PR;
  • added the passthru attribute, including the libDir as per your suggestion;
  • changed the gem/lib folders to use the version.libDir logic from ruby-version.nix, inline with what bundler does;
  • added a devEnv attribute to passthru, like the one in the MRI package. This is accomplished by putting the derivation into a let clause, and using overrideAttrs.

Happy to make any further changes as required.

zimbatm
zimbatm approved these changes Jul 19, 2017
View reviewed changes
Copy link
Member

@zimbatm zimbatm left a comment

Choose a reason for hiding this comment

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

LGTM. Nice work on this PR!

@zimbatm zimbatm merged commit 4af15b5 into NixOS:master Jul 19, 2017
@cstrahan
Copy link
Contributor

Awesome work, @asppsa! Let me know if you have any problems with bundlerEnv, and I'll try to help out where I can.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zimbatm zimbatm

@0xABAB 0xABAB

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@asppsa @cstrahan @zimbatm @0xABAB