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.

Sign up for GitHub

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

stups: init at 1.1.21 #71389

Merged
merged 4 commits into from Dec 8, 2019
Merged

stups: init at 1.1.21 #71389

merged 4 commits into from Dec 8, 2019

Conversation

mschuwalow
Copy link
Contributor

Motivation for this change

Add derivations for https://github.com/zalando-stups/stups-cli

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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @

jonringer
jonringer reviewed Oct 19, 2019
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

Also, it should be roughly 1 commit for one package (think in terms of cherry-picking commits across branches). It should look like:

maintainers: add mschuwalow
stups-berry: init at 1.0.28
stups-cli-support: init at 1.1.20
...

pkgs/development/python-modules/stups-berry/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/stups-cli-support/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/stups-cli-support/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/stups-fullstop/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/stups-tokens/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/stups/default.nix Outdated Show resolved Hide resolved
pkgs/top-level/python-packages.nix Outdated Show resolved Hide resolved
@mschuwalow
Copy link
Contributor Author

Also, it should be roughly 1 commit for one package (think in terms of cherry-picking commits across branches). It should look like:

maintainers: add mschuwalow
stups-berry: init at 1.0.28
stups-cli-support: init at 1.1.20
...

Ok. Is it alright to contribute them as part of one pr?

@jonringer
Copy link
Contributor

yea, same PR, just many commits

@mschuwalow mschuwalow force-pushed the feature/add-stups branch 4 times, most recently from 4a65edd to 743f63e Compare October 19, 2019 17:11
@jonringer
Copy link
Contributor

you're welcome to have all the commits in one PR :), in general, it just takes a little longer to resolve all the issues.

@mschuwalow mschuwalow force-pushed the feature/add-stups branch 2 times, most recently from e1b5324 to 3aaace9 Compare October 19, 2019 17:46
@mschuwalow
Copy link
Contributor Author

mschuwalow commented Oct 19, 2019 •
edited

you're welcome to have all the commits in one PR :), in general, it just takes a little longer to resolve all the issues.

I agree that it makes sense to split them up. I have included derivations for 3 packages here and they seem to be passing.

Would be great if you could do another around of review and check whether they now conform to the guidelines now. If they do, feel free to merge this and I'll do the other packages separately.

@risicle
Copy link
Contributor

risicle commented Oct 20, 2019

The test phases for all 3 packages go downloading random dependencies from pypi, certainly on macos 10.13. These dependencies instead need to be provided through nix via checkInputs.

@risicle
Copy link
Contributor

risicle commented Oct 20, 2019

Oh also this does not work on python2 - you'll have to disable it for !isPy3k.

@mschuwalow
Copy link
Contributor Author

If updated the pr. Thank you both for your responsiveness and patience 💪

@mschuwalow
Copy link
Contributor Author

could you give this another review / list what needs to be done to get this merged?

jonringer
jonringer reviewed Nov 6, 2019
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

In general, the most common layout for a nix expression follows the order in which it evaluated then built:

the top-level attrs should roughly be:

pname
version
[disabled]
src
nativeBuildInputs
buildInputs
propagatedBuildInputs
patches
buildPhase + hooks
installPhase + hooks
checkPhase + hooks (and maybe checkInputs)
meta

checkPhase and checkInputs can be grouped together, but this is roughly the paradigm adhered to by most packages.

pkgs/development/python-modules/stups-tokens/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/stups-zign/fix_tests.patch Outdated Show resolved Hide resolved
@mschuwalow
Copy link
Contributor Author

@jonringer could you take another look?

@mschuwalow
Copy link
Contributor Author

@jonringer I've addressed your remarks

@mschuwalow
Copy link
Contributor Author

@jonringer Sorry for bothering you, but I would like to get this merged.
What remains to be done here?

jonringer
jonringer approved these changes Nov 25, 2019
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

nix-review passes on NixOS
diff LGTM
commits LGTM

[11 built, 11 copied (3.4 MiB), 0.7 MiB DL]
https://github.com/NixOS/nixpkgs/pull/71389
6 package were built:
python37Packages.stups-cli-support python37Packages.stups-tokens python37Packages.stups-zign python38Packages.stups-cli-support python38Packages.stups-tokens python38Packages.stups-zign

Lassulus
Lassulus approved these changes Dec 8, 2019
View reviewed changes
Copy link
Member

@Lassulus Lassulus left a comment

Choose a reason for hiding this comment

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

tested with nix-review, seems to be fine

@Lassulus Lassulus merged commit 074f444 into NixOS:master Dec 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Lassulus Lassulus Lassulus approved these changes

@jonringer jonringer jonringer approved these changes

@FRidh FRidh Awaiting requested review from FRidh

Assignees
No one assigned
Labels
6.topic: python 8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer 12. first-time contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@mschuwalow @jonringer @risicle @Lassulus @Janik-Haag