ruby: rubygems 2.7.7 -> 3.0.3 #60394
Conversation
There was a problem hiding this comment.
nice work. Just one comment and ofborg eval issue with the license field:
while evaluating 'handleEvalIssue' at /var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/mr-est/eval-0-gleber.ewr1.nix.ci/.gc-of-borg-outpaths.nix:26:37, called from /var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/mr-est/eval-0-gleber.ewr1.nix.ci/pkgs/stdenv/generic/check-meta.nix:158:8:
evaluation aborted with the following error message: 'Failed to evaluate rubygems: «unknown-meta»: has an invalid meta attrset:
- key 'licenses' is unrecognized; expected one of:
['available', 'badPlatforms', 'branch', 'broken', 'description', 'downloadPage', 'downloadURLRegexp', 'executables', 'homepage', 'hydraPlatforms', 'isBuildPythonPackage', 'isFcitxEngine', 'isGutenprint', 'isIbusEngine', 'knownVulnerabilities', 'license', 'longDescription', 'maintainers', 'name', 'outputsToInstall', 'platforms', 'position', 'priority', 'repositories', 'schedulingPriority', 'tag', 'tests', 'timeout', 'updateWalker', 'version']'
| patches = [ | ||
| ./0001-add-post-extract-hook.patch | ||
| ./0002-binaries-with-env-shebang.patch | ||
| ./0003-gem-install-default-to-user.patch |
There was a problem hiding this comment.
in general we try to use fetchPatch when possible to avoid growing the nixpkgs repo unnecessarily
There was a problem hiding this comment.
IMO "when possible" shouldn't include pointing to patch URL's on a single maintainer's GitHub fork – if this were the case, Nixpkgs wouldn't need to contain any patch files at all, but that's clearly not how things are?
There was a problem hiding this comment.
To be fair I don't think we have a clear policy on that and it's probably not enforced consistently across nixpkgs.
Patches should be cached by the binary cache so having them on a fork shouldn't really be a problem.
There was a problem hiding this comment.
Would it be better for you if we maintained the fork in the nix-community org?
Rather than rolling our own. This means that we can use all of the extra functionality stdenv gives us, like being able to provide a list of patches rather than just one.
I think it makes more sense to keep the patches in-tree than on zimbatm's RubyGems fork.
|
Would it be better for you if we maintained the fork in the nix-community org?
A little, I guess, but not really?
It just feels wrong to create a whole new repository for the sake of a few /tiny/ files in Nixpkgs. With a seperate repository it becomes harder to see changes to these files, harder to update them when they don't apply (_especially_ for contributors without commit access to the RubyGems fork), and just generally more ceremony to maintain.
If there were a hundred patches, or if they were huge, or if there were some canonical source for them like an upstream bug report, I could see a case for keeping them external, but it doesn't feel right to do so for a few tiny, Nixpkgs-specific changes.
|
|
Do you want to put this on staging, or is master fine? I've merged my larger package updates (like git) in staging and this seems to be approaching the "mass rebuild" category. |
|
@alyssais if the patches don't apply cleanly on the new branch then it's a bit easier to create a new branch and rebase the patches onto upstream's release in my experience. Assuming that the maintainer has access to the fork. Since you are the maintainer in that case, I think the minimum would be for you to get access to the fork, or relocate the fork in a good location. And if you prefer to inline the patches inside of nixpkgs then I'm happy to defer to your opinion as well. |
This is nowhere near the size of a git upgrade, but if there's any question about whether something should go to staging, that's probably a good sign it should go to staging. :) |
ghost
mentioned this pull request
|
I still think the patches are better inline, and so I am going to go ahead with that. |
With this change, we can finally easily upgrade Rubygems, and get off this ancient version. Patches no longer have to live on @zimbatm’s GitHub and be applied using a hack that can only handle a single patch. Hooray!
Attempt 2, following #57809.
Things done
sandboxinnix.confon non-NixOS)nix-shell -p nix-review --run "nix-review wip"./result/bin/)nix path-info -Sbefore and after)