bundlerEnv: add platforms + support `gemspec` #23000

nyarly · 2017-02-19T18:31:38Z

Motivation for this change

Ruby Gemfiles can use a gemspec directive, which includes the dependencies of the gem specification into the Gemfile. For this to work, the contents of the gem directory need to get copied into the gemfile-and-lockfile derivation so that bundler can determine those dependencies. (Yes, the gempsec files often include code from arbitrary places in the gem (e.g. to determine the version of the gem), so it has to be all the files.)

Furthermore, Gemfiles can specify that certain gems are only needed when running on certain Ruby VMs (e.g. rubysdl-* is needed on Rubinius), which can lead to broken Nix builds if we try to bring in all the gems mentioned in the Gemfile.

Finally, this supports the use of "path" gem sources. These should probably not be allowed in distributed gems but are very handy for gem development. They allow a Gemfile to refer to a gem on the local filesystem (including the actual "root" gem for the Gemfile - so this is a requirement of gemspec Gemfiles as well.) This is useful when developing two closely related gems e.g. a rspec or rails plugin, where tweaking the host gem is helpful for debugging.

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 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.

mention-bot · 2017-02-19T18:31:39Z

@nyarly, thanks for your PR! By analyzing the history of the files in this pull request, we identified @abbradar, @zimbatm and @manveru to be potential reviewers.

nyarly · 2017-02-19T18:36:06Z

I maybe chose a difficult first PR - bundlerEnv is (mostly) an intermediate builder for lots of gems, so e.g. it doesn't have any ./result/bin of its own, but plenty of other outs rely on it working correctly. I can confirm it continues to work with jekyll and now works with shex, and I'm interested in sticking around to see this merged.

Not every gem package uses pname, nor should it.

zimbatm

Great! I left a few comments but overall it looks good so don't get discouraged by them and don't feel obligated to implement all of them.

zimbatm · 2017-02-19T18:45:27Z

pkgs/development/ruby-modules/bundler-env/default.nix

+  !(attrs ? "platforms") ||
+    builtins.any (platform:
+      platform.engine == ruby.rubyEngine &&
+        (!(platform ? "version") || platform.version == ruby.version.majMin)


a note to myself: make sure these are the right version that are being matched. On MRI is't 1-1 but not on jruby for example.

Certainly there'll need to be a mapping from Gemfile platform version to NixOS ruby.version.majMin. I'm not sure the two should be identical. At the moment, Bundix doesn't yet include this information in the gemset.nix (although that's what I'm up to now...) so it's not pressing.

I wonder if the best solution isn't to provide a mapping function in bunder-env itself?

zimbatm · 2017-02-19T18:48:58Z

pkgs/development/ruby-modules/bundler-env/default.nix

+  gems = lib.flip lib.mapAttrs configuredGemset (name: attrs: buildGem name attrs);
+
+  maybeCopyAll = { usesGemspec ? false, ...}@main:
+  (if usesGemspec then ''


nit: parenthesis not needed

zimbatm · 2017-02-19T18:49:26Z

pkgs/development/ruby-modules/bundler-env/default.nix

+
+  maybeCopyAll = { usesGemspec ? false, ...}@main:
+  (if usesGemspec then ''
+  cp -a ${gemdir}/* $out/


nit: this can be a single-line string

zimbatm · 2017-02-19T18:50:28Z

pkgs/development/ruby-modules/bundler-env/default.nix

  '';

  envPaths = lib.attrValues gems ++ lib.optional (!hasBundler) bundler;

-  binPaths = if mainGem != null then [ mainGem ] else envPaths;
+  binPaths = if mainGem != null then [ mainGem ] ++ envPaths else envPaths;


can you explain the reasoning behind that change?

The summary is: I think the requirements here are ambiguous and I needed this interpretation.

My primary motivation here has been in cases where I'm developing a gem or ruby app, and so I want to use bundlerEnv in shell.nix. In that case, the Gemfile often includes a bunch of gems that are incltuded for use in development - and if there's an executable in the main gem, I want to be able to run it as well.

This may be in tension with the case where we're installing a ruby app with nix-env, so we don't want to pull in e.g. rspec as a executable. (As an aside, IMO if you're distrubing a ruby executable, then you distribute a gem, so nixpkgs should use buildRubyGem... but that's not even 100% from a ruby-community standpoint.)

I haven't confirmed this, but there's also the significant use case of shipping Rails apps, and they absolutely assume a git repo checked out and bundled in place, so there's definitely a use case for bundlerEnv for applications and therefore avoiding collisions with rspec. That said though, those rails apps will have bin directories with the same set of names...

The upshot is probably that the conditional here shouldn't be "mainGem != null" - but I don't know what the conditional should be.

The issue is that this will link bundler into the bin/ folder. And this will introduce conflicts when two projects are going to be installed.

The runtime (or nix-shell) should have access to $GEM_HOME/bin where all the runtime dependencies are installed. Or we need another layer for the wrapped gem bins.

As I understand this better: why would we add any executable apart from the main gem's? If I were foolish enough to nix-env -i a library gem whose Gemfile brought in Rake or RSpec, we'd have the same problem, right?

Yeah we shouldn't link any binaries unless they are part of the main gem.

There is a bit of transition issue because the pname attribute wasn't there initially so it was not possible to know what was the main gem.

zimbatm · 2017-02-19T18:52:40Z

pkgs/development/ruby-modules/bundler-env/default.nix

+      });
+    in
+    if gemAttrs.type == "path" then pathDerivation gemAttrs
+    else buildRubyGem gemAttrs);


It would be nice if this could be pushed into the gem derivation

I debated that. I'm still soft on my understanding of how path-gems should get handled, which led me to hedge a little.

zimbatm · 2017-02-19T18:55:35Z

pkgs/development/ruby-modules/bundler-env/default.nix

+          out = res;
+          outputName = "out";
+        };
+    in res;


I'm a bit confused by this, shouldn't it be wrapped in a derivation?

The problem being solved here is the use of gems that are on the local filesystem e.g. not in the nix store. So this wrapper is a kind of fake derivation to make that possible.

One thing I'm not sure how to do, but I think needs to be added is that these "derivations" need to taint their dependent - it can be used as a shell.nix, but there's no way it should be put into nixpkgs like this.

There's one exception here, and that's the mainGem, because its files (should) get copied into the nix store as part of gemfile-and-lockfile.

zimbatm · 2017-02-19T18:56:58Z

pkgs/development/ruby-modules/bundler-env/default.nix

+
+  maybeCopyAll = main: if main == null then "" else copyIfUseGemspec main;
+
+  copyIfUseGemspec = { usesGemspec ? false, ...}@main:


This is also true in the cases where gem "name", path: "some/dir" is being used. I think we can make the assumption that the path should be under the same folder.

Re-reading this, I actually think that assumption is likely to be wrong - at least in my experience it would be. I've most needed to use the Gemfile :path => ... syntax when I've had two gems that were tightly coupled (e.g. rspec plugins) and needed to get them to "meet in the middle" on some issue.

However, in a "production" situation, path gems being siblings in a git repo is a very fair assumption.

Of course, if I'd read with a little more context...

Yes, any mainGem that has a path source is going to need everything copied in. Furthermore, because of how Bundler handles gemspec in the first place, the two kinds of gems are indistinguishable (Bundler parses a Gemfile/lock with gemspec as if it were gem "name", :path => ., effectively.)

nyarly · 2017-02-19T22:51:19Z

@zimbatm Not discouraged by comments! And not unwilling to make changes, but I'd like to make sure I'm headed in the right direction with them.

`usesGemspec` no longer required to trigger the "copy everything into gemfile-and-lock" behavior. If the mainGem is referred to by path, that's sufficient.

nyarly · 2017-02-21T05:07:09Z

@zimbatm - I'd like to hash out some of the details of this; I'm in UTC-8, though, and I'll be in transit tomorrow morning, which looks like our best overlap.

zimbatm · 2017-02-22T19:26:43Z

@nyarly yeah good idea. I will be away for a bit. Can you send me a calendar invite that would work between 10 am to 10 pm UTC-0 starting Wed next week?

zimbatm · 2017-03-10T08:24:30Z

Still want to do this? I'm pretty free next week

…

On Tue, 21 Feb 2017, 05:07 Judson Lester, ***@***.***> wrote: @zimbatm <https://github.com/zimbatm> - I'd like to hash out some of the details of this; I'm in UTC-8, though, and I'll be in transit tomorrow morning, which looks like our best overlap. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#23000 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAMsJShkLg_HETEtp-DIMw6vb3WRUxlks5renEAgaJpZM4MFi7v> .

nyarly · 2017-03-13T05:56:42Z

I'm still interested, but I need to figure out a time that aligns with you. I'll be in touch soon.

…

On Fri, Mar 10, 2017 at 12:24 AM zimbatm ***@***.***> wrote: Still want to do this? I'm pretty free next week On Tue, 21 Feb 2017, 05:07 Judson Lester, ***@***.***> wrote: > @zimbatm <https://github.com/zimbatm> - I'd like to hash out some of the > details of this; I'm in UTC-8, though, and I'll be in transit tomorrow > morning, which looks like our best overlap. > > — > You are receiving this because you were mentioned. > > Reply to this email directly, view it on GitHub > <#23000 (comment)>, or mute > the thread > < https://github.com/notifications/unsubscribe-auth/AAAMsJShkLg_HETEtp-DIMw6vb3WRUxlks5renEAgaJpZM4MFi7v > > . > — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#23000 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHyPEseSpKo0g4Df14G5wM0n341FW_sks5rkQjBgaJpZM4MFi7v> .

zimbatm · 2017-03-23T16:40:00Z

Feel free to send me an email so we can setup a time :)

The bin stubs need to be built where there's access to /nix/store - so it can't happen in a nix-shell run. Ergo, a shell.nix needs to be able to signal to the build that all bins need to be built.

nyarly · 2017-04-09T22:50:37Z

Following discussion with @zimbatm and some reflection of my own, I'm closing this in favor of a to-come PR.

We agreed that the existing bundlerEnv was too complex, primarily because it's trying to serve too many purposes. The use cases we identified were:

"tool" gems, which from a Nix user's perspective are just another executable, like TaskJuggler or Jekyll.
Rails apps, which need to set up an execution environment, but not put things into the path, like GitLab.
nix-shell development environments for either of the above, plus library gems.

All three of these general use cases were using the same bundlerEnv expression, and as a result it's accreted a lot of configuration over time.

Rather than contribute further to that weird configuration, the plan is to produce separate expressions for different use cases, ideally with common behavior split out into its own expression.

nyarly added 2 commits February 19, 2017 10:18

Updates to bundlerEnv

c765f68

Merge remote-tracking branch 'nixos/master'

2b9c7b4

Need to handle "null" mainGems

3c99411

Not every gem package uses pname, nor should it.

zimbatm reviewed Feb 19, 2017

View reviewed changes

Simplifying interface on gemset.nix slightly.

0481a33

`usesGemspec` no longer required to trigger the "copy everything into gemfile-and-lock" behavior. If the mainGem is referred to by path, that's sufficient.

nyarly added 2 commits March 26, 2017 17:32

Adding "allBins" flag on bundlerEnv

7f6e8a1

The bin stubs need to be built where there's access to /nix/store - so it can't happen in a nix-shell run. Ergo, a shell.nix needs to be able to signal to the build that all bins need to be built.

Merge remote-tracking branch 'nixos/master'

ed77e11

nyarly closed this Apr 9, 2017

nyarly mentioned this pull request May 21, 2017

BundlerEnv, now with groups and paths #25980

Merged

Janik-Haag added the 12. first-time contribution label Jun 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bundlerEnv: add platforms + support `gemspec` #23000

bundlerEnv: add platforms + support `gemspec` #23000

nyarly commented Feb 19, 2017 •

edited

mention-bot commented Feb 19, 2017

nyarly commented Feb 19, 2017

zimbatm left a comment

zimbatm Feb 19, 2017

nyarly Feb 19, 2017

zimbatm Feb 19, 2017

zimbatm Feb 19, 2017

zimbatm Feb 19, 2017

nyarly Feb 19, 2017

zimbatm Feb 22, 2017

nyarly Mar 23, 2017

zimbatm Mar 23, 2017

zimbatm Feb 19, 2017

nyarly Feb 19, 2017

zimbatm Feb 19, 2017

nyarly Feb 19, 2017

zimbatm Feb 19, 2017

nyarly Feb 19, 2017

nyarly Feb 21, 2017

nyarly commented Feb 19, 2017

nyarly commented Feb 21, 2017

zimbatm commented Feb 22, 2017

zimbatm commented Mar 10, 2017 via email

nyarly commented Mar 13, 2017 via email

zimbatm commented Mar 23, 2017

nyarly commented Apr 9, 2017


		maybeCopyAll = main: if main == null then "" else copyIfUseGemspec main;

		copyIfUseGemspec = { usesGemspec ? false, ...}@main:

bundlerEnv: add platforms + support gemspec #23000

bundlerEnv: add platforms + support gemspec #23000

Conversation

nyarly commented Feb 19, 2017 • edited

Motivation for this change

Things done

mention-bot commented Feb 19, 2017

nyarly commented Feb 19, 2017

zimbatm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nyarly commented Feb 19, 2017

nyarly commented Feb 21, 2017

zimbatm commented Feb 22, 2017

zimbatm commented Mar 10, 2017 via email

nyarly commented Mar 13, 2017 via email

zimbatm commented Mar 23, 2017

nyarly commented Apr 9, 2017

bundlerEnv: add platforms + support `gemspec` #23000

bundlerEnv: add platforms + support `gemspec` #23000

nyarly commented Feb 19, 2017 •

edited