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

ruby: add ruby.withPackages #61114

Merged
merged 1 commit into from Sep 3, 2019
Merged

ruby: add ruby.withPackages #61114

merged 1 commit into from Sep 3, 2019

Conversation

@manveru
Copy link
Contributor

manveru commented May 7, 2019

Motivation for this change

This is mostly open for discussion, it allows usage of ruby.withPackages similar to the way python and haskell work.

It also includes ruby.gems, with corresponding ruby_2_3-gems ruby_2_4-gems ruby_2_5-gems ruby_2_6-gems attributes in top-level to allow hydra to compile them.

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

@zimbatm

This comment has been minimized.

Copy link
Member

zimbatm commented May 8, 2019

It's a good idea. We should have all the gems that are referenced in the defaultGemConfig so their code gets exercised.

@nixos-discourse

This comment has been minimized.

Copy link

nixos-discourse commented May 9, 2019

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/what-are-your-goals-for-19-09/2875/5

@nixos-discourse

This comment has been minimized.

Copy link

nixos-discourse commented May 10, 2019

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/ruby-gem-typhoeus-could-not-open-library-libcurl/2677/11

Copy link
Member

zimbatm left a comment

looking good!

Copy link
Member

alyssais left a comment

This is awesome. Thanks for the work.

  • I think ruby-packages.nix would make more sense as pkgs/development/ruby-modules/with-packages/gemset.nix.

  • Do we really have to expose a top-level attribute to get hydra to build the gems? That’s annoying…

@manveru

This comment has been minimized.

Copy link
Contributor Author

manveru commented May 15, 2019

I mirrored the structure of Haskell/Idris/Lua/Python for the with-packages naming, so I think that's the right location. And yes, Hydra will not look into passthru for things to build, so we have to expose it explicitly.

attrs = functions.applyGemConfigs ({ inherit ruby; gemName = name; } // initialAttrs);
in
buildRubyGem (functions.composeGemAttrs ruby gems name attrs)
) (import ../../../top-level/ruby-packages.nix);

This comment has been minimized.

Copy link
@zimbatm

zimbatm May 16, 2019

Member

What is the advantage of creating so much distance between the producer and the consumer of the gems.nix file?

Even the updater script could be moved into this folder so everything is co-located.

This comment has been minimized.

Copy link
@manveru

manveru May 21, 2019

Author Contributor

For the ruby-packages.nix I follow the pattern of all the other languages that put them in top-level. I agree that it's a bit awkward to have them so separated, but I didn't really want to clutter the top-level directory with too much ruby specific stuff.
I can maybe invoke update-ruby-packages from the /maintainers/scripts and put the actual code in the with-packages directory. Again, I tried to be consistent with update-python-packages update-luarocks-packages.

@alyssais

This comment has been minimized.

Copy link
Member

alyssais commented May 17, 2019

I mirrored the structure of Haskell/Idris/Lua/Python for the with-packages naming, so I think that's the right location.

This makes sense to me, although if we’re doing that we might want to mirror the rubyPackages_2_5 convention for the top-level attributes for consistency there as well.

Copy link
Member

alyssais left a comment

I’d like the defaultGemConfig changes to be in their own commit.

'';
};
fog-dnsimple = attrs:
if lib.versionOlder attrs.version "1.0.1" then {

This comment has been minimized.

Copy link
@alyssais

alyssais May 17, 2019

Member

Suggestion: lib.optionalAttrs

FileUtils.mkdir_p(tmpdir)

failing = test_cases.reject do |name, test_case|
test_case = <<-SHELL

This comment has been minimized.

Copy link
@alyssais

alyssais May 17, 2019

Member
Suggested change
test_case = <<-SHELL
test_case = <<~SHELL

Will let you indent the below nicely.

This comment has been minimized.

Copy link
@manveru

manveru May 21, 2019

Author Contributor

Yeah, my rubocop somehow is stuck at 2.2 or 2.3 and doesn't accept the syntax. I think we can remove this file anyway since we have the test.nix

@nixos-discourse

This comment has been minimized.

Copy link

nixos-discourse commented May 22, 2019

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/ruby-exposing-new-gems-to-a-bundlerapp-installed-tool/2989/3

@FRidh

This comment has been minimized.

Copy link
Member

FRidh commented Jun 18, 2019

What's the status of this PR?

@manveru manveru force-pushed the manveru:ruby-with-packages branch from 99a79d4 to 80ef88e Jun 18, 2019
@FRidh

This comment has been minimized.

Copy link
Member

FRidh commented Jun 18, 2019

Alright. The docs look really good!

Copy link
Member

alyssais left a comment

These docs are amazing. Some of the best I’ve seen for nixpkgs.

In addition to these comments, it looks like a couple of mine from previous reviews are still outstanding.

@shepting

This comment has been minimized.

Copy link

shepting commented Aug 7, 2019

I'm pumped to see this land!

@manveru manveru force-pushed the manveru:ruby-with-packages branch from 61cc0b5 to 2b9a394 Sep 2, 2019
@FRidh FRidh added this to the 19.09 milestone Sep 2, 2019
@FRidh

This comment has been minimized.

Copy link
Member

FRidh commented Sep 2, 2019

What is the status? I think it would be good if this could go in before branch-off Saturday the 7th. Note that due to the rebuilds it also needs to go via staging which takes extra time.

@alyssais

This comment has been minimized.

Copy link
Member

alyssais commented Sep 3, 2019

I’m going to make some small doc tweaks and then merge, probably today.

@FRidh

This comment has been minimized.

Copy link
Member

FRidh commented Sep 3, 2019

Actually, it will be tight to get it into staging, so just send it to master. It's less than 2500 rebuilds so it's not that bad.

@alyssais alyssais force-pushed the manveru:ruby-with-packages branch from 9c6b446 to 6106a62 Sep 3, 2019
Co-authored-by: Alyssa Ross <hi@alyssa.is>
@alyssais alyssais force-pushed the manveru:ruby-with-packages branch from 6106a62 to 28d7e50 Sep 3, 2019
@alyssais alyssais merged commit 1f49035 into NixOS:master Sep 3, 2019
1 check was pending
1 check was pending
grahamcofborg-eval Cloning project
Details
@vcunat vcunat mentioned this pull request Dec 10, 2019
4 of 10 tasks complete
@marsam marsam mentioned this pull request Dec 12, 2019
3 of 10 tasks complete
@manveru manveru deleted the manveru:ruby-with-packages branch Dec 12, 2019
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.