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

Jump to bottom

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 manveru commented May 7, 2019 •
edited

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
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
Copy link

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
Copy link

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

zimbatm
zimbatm reviewed May 10, 2019
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.

looking good!

pkgs/development/ruby-modules/with-packages/default.nix Show resolved Hide resolved
pkgs/development/ruby-modules/with-packages/test.nix Show resolved Hide resolved
pkgs/development/ruby-modules/with-packages/test.nix Show resolved Hide resolved
alyssais
alyssais reviewed May 12, 2019
View reviewed changes
Copy link
Member

@alyssais alyssais left a comment

Choose a reason for hiding this comment

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

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…

pkgs/development/ruby-modules/with-packages/default.nix Outdated Show resolved Hide resolved
pkgs/development/ruby-modules/with-packages/test.nix Show resolved Hide resolved
pkgs/development/ruby-modules/with-packages/test.nix Outdated Show resolved Hide resolved
pkgs/development/ruby-modules/with-packages/test.nix Outdated Show resolved Hide resolved
@manveru
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.

zimbatm
zimbatm reviewed May 16, 2019
View reviewed changes
pkgs/development/ruby-modules/with-packages/default.nix Outdated
attrs = functions.applyGemConfigs ({ inherit ruby; gemName = name; } // initialAttrs);
in
buildRubyGem (functions.composeGemAttrs ruby gems name attrs)
) (import ../../../top-level/ruby-packages.nix);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Member

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.

alyssais
alyssais reviewed May 17, 2019
View reviewed changes
Copy link
Member

@alyssais alyssais left a comment

Choose a reason for hiding this comment

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

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

pkgs/development/ruby-modules/gem-config/default.nix Outdated
'';
};
fog-dnsimple = attrs:
if lib.versionOlder attrs.version "1.0.1" then {
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: lib.optionalAttrs

pkgs/development/ruby-modules/with-packages/test.rb Outdated
FileUtils.mkdir_p(tmpdir)

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

Choose a reason for hiding this comment

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

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

Will let you indent the below nicely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

pkgs/development/ruby-modules/with-packages/default.nix Show resolved Hide resolved
@nixos-discourse
Copy link

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
Copy link
Member

FRidh commented Jun 18, 2019

What's the status of this PR?

@FRidh
Copy link
Member

FRidh commented Jun 18, 2019

Alright. The docs look really good!

alyssais
alyssais requested changes Jun 18, 2019
View reviewed changes
Copy link
Member

@alyssais alyssais left a comment

Choose a reason for hiding this comment

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

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.

doc/languages-frameworks/ruby.section.md Show resolved Hide resolved
doc/languages-frameworks/ruby.section.md Show resolved Hide resolved
@alyssais alyssais mentioned this pull request Jun 18, 2019
Merged
10 tasks
@ofborg ofborg bot requested a review from vrthra June 19, 2019 01:07
@shepting
Copy link

shepting commented Aug 7, 2019

I'm pumped to see this land!

@alyssais alyssais mentioned this pull request Aug 12, 2019
Closed
@FRidh FRidh added this to the 19.09 milestone Sep 2, 2019
@FRidh
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
Copy link
Member

alyssais commented Sep 3, 2019

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

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

@manveru @alyssais
ruby.withPackages: init
28d7e50 
Co-authored-by: Alyssa Ross <hi@alyssa.is>
@alyssais alyssais merged commit 1f49035 into NixOS:master Sep 3, 2019
@vcunat vcunat mentioned this pull request Dec 10, 2019
Merged
10 tasks
@marsam marsam mentioned this pull request Dec 12, 2019
Closed
10 tasks
@manveru manveru deleted the ruby-with-packages branch December 12, 2019 17:14
@zakame zakame mentioned this pull request Oct 21, 2020
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zimbatm zimbatm zimbatm left review comments

@alyssais alyssais alyssais requested changes

@vrthra vrthra Awaiting requested review from vrthra

Assignees
No one assigned
Labels
6.topic: ruby 8.has: documentation 8.has: package (new) 10.rebuild-darwin: 0 10.rebuild-linux: 0 11.by: package-maintainer
Projects
None yet
Milestone
19.09
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@manveru @zimbatm @nixos-discourse @alyssais @FRidh @shepting