-
-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
csvkit: fix build #97517
csvkit: fix build #97517
Conversation
sorry, I saw the other first, I think a fix already went it for the commit message, please have it adhere to CONTRIBUTing:
ZHF is just a period of time |
77f0d3e
to
bd4b242
Compare
I saw the merge conflicts, and hence the changes performed by @zookatron at 32c9ee2 . TBH, I think my changes are better. I rebased it all upon the current master. All of the packages used in my version are of their latest versions, and there are no overrides at all used. An upstream PR was fetched so all tests should pass, plus a version bump.
I do adhere to it. The PR includes 4 commits each dealing with a different package and the PR title is just the name of the branch. |
Thanks for taking the time to update these packages more thoroughly @doronbehar. For my changes I was just trying to make the minimal changes necessary for Zero Hydra Failures and I did not update the package version itself as my understanding is that we should avoid updating the package versions themselves when back-porting changes to an already stable branch. |
}; | ||
|
||
patches = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add a comment as to why the patch is necesary, and when it's likely to be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍.
meta = with stdenv.lib; { | ||
homepage = "http://www.sqlalchemy.org/"; | ||
description = "A Python SQL toolkit and Object Relational Mapper"; | ||
license = licenses.mit; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would really like to have a maintainer of this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, myself. I also updated the whole meta attribute as this one was of a copy paste.
bd4b242
to
d1e18dd
Compare
Are we still interested in this @jonringer ? |
I think for master, this is fine, but for release-20.09 it's adding a new package, and affecting some others. I was on vacation and only had a laptop, so I couldn't do large rebuilds |
d1e18dd
to
444e845
Compare
actually, seeing as the package addition was to fix a different build, maybe it should be included |
Remove unneeded glibcLocales. Remove overrided agate-sql and agate-dbf, as these overrides are not needed. Use pytestCheckHook instead of overriding checkPhase. Add an upstream patch that fixes tests.
444e845
to
77bd374
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Diff LGTM
- Commits LGTM
- Builds
https://github.com/NixOS/nixpkgs/pull/97517
5 packages built:
csvkit python37Packages.agate-sql python37Packages.crate python38Packages.agate-sql python38Packages.crate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Result of nixpkgs-review pr 97517 1
5 packages built:
- csvkit
- python37Packages.agate-sql
- python37Packages.crate
- python38Packages.agate-sql
- python38Packages.crate
Motivation for this change
#97479 cc @NixOS/nixos-release-managers .
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)