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

piccata 1.0.1 -> 2.0.0 #90105

Merged
merged 1 commit into from Jun 12, 2020
Merged

Conversation

siriobalmelli
Copy link
Contributor

Motivation for this change

Needed piccata working with Python3, upgraded to latest version.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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.

NOTE:

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.

LGTM

failures are broken on target branch

https://github.com/NixOS/nixpkgs/pull/90105
2 packages failed to build:
nrfutil python27Packages.piccata

2 packages built:
python37Packages.piccata python38Packages.piccata

@siriobalmelli
Copy link
Contributor Author

LGTM

failures are broken on target branch

https://github.com/NixOS/nixpkgs/pull/90105
2 packages failed to build:
nrfutil python27Packages.piccata

2 packages built:
python37Packages.piccata python38Packages.piccata

Thanks for the review :)

Yes, as mentioned above:

I did get nrfutil building, but there are quite a few patches required to get there.
Not sure if you'd prefer to see all patches in a single pull request, but right now I have every patch broken out as a separate branch, and I've filed a PR for each of them:

... and then once this PR and the above are merged, I would submit the remaining patches:

... all so I can finally fix nrfutil:

Again, pls let me know if you'd like to see all patches together or if I should follow the current plan.
Thanks.

@jonringer
Copy link
Contributor

@siriobalmelli please apply my suggestion under the approval, so that the python2 package fails at evaluation time, instead of build time.

Otherwise LGTM

@jonringer
Copy link
Contributor

Please follow CONTRIBUTING.md and manual#submitting-changes-making-patches and squash the commits.

This can be done with:

git reset HEAD~2
git add -- pkgs/
git commit --amend --no-edit
git push ... ... --force

Signed-off-by: Sirio Balmelli <sirio@b-ad.ch>
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.

LGTM

Result of nixpkgs-review pr 90105 1

2 packages built:
- python37Packages.piccata
- python38Packages.piccata

@jonringer jonringer merged commit 803ef3f into NixOS:master Jun 12, 2020
@siriobalmelli siriobalmelli deleted the upgrade/piccata branch June 13, 2020 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants