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

pgcli: 1.11.0 -> 2.0.0 #49853

Merged
merged 4 commits into from Nov 10, 2018
Merged

pgcli: 1.11.0 -> 2.0.0 #49853

merged 4 commits into from Nov 10, 2018

Conversation

marsam
Copy link
Contributor

@marsam marsam commented Nov 7, 2018 •
edited

Motivation for this change
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 nox --run "nox-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)
  • Fits CONTRIBUTING.md.

@marsam marsam requested a review from FRidh as a code owner November 7, 2018 03:59
@Ma27
Copy link
Member

Ma27 commented Nov 7, 2018

Currently running nix-review on your branch to see if anything else explicitly breaks.

Ma27
Ma27 requested changes Nov 7, 2018
View reviewed changes
Copy link
Member

@Ma27 Ma27 left a comment

Choose a reason for hiding this comment

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

Especially major bumps of Python modules in nixpkgs are unfortunately quite error-prone.
I identified the following packages that seem to break with prompt_toolkit bumped to 2.x:

  • pkgs.haxor-news
  • pkgs.pythonPackages.ptpython
  • pkgs.xonsh (pygments is missing, this may be happening because of the removed pygments in prompt_tools, but in that case it's certainly an issue of xonsh)
  • pkgs.python3Packages.jupyterhub (same as above)

It would be great if you could fix these packages (or provide pythonPackages.prompt_tools and pythonPackages.prompt_tools2 temporarily until all affected packages support v2).

Feel free to ping me if I can help you :)

@marsam marsam force-pushed the feature/update-pgcli branch 2 times, most recently from 8446539 to 8aaf371 Compare November 9, 2018 13:55
@marsam
Copy link
Contributor Author

marsam commented Nov 9, 2018

@Ma27 thanks for your review, I didn't know about nix-review. I believe I've fixed the errors

  • xonsh: 0.6.8 -> 0.8.3 : tested on NixOS and Darwin
  • pythonPackages.ptpython: 0.41 -> 2.0.4 : tested on NixOS and Darwin

also ipython5 and ipython6 only work with prompt_toolkit 1.x and don't have plans to upgrade to prompt_toolkit 2. ipython7 (not yet in nixpkgs) works prompt_toolkit 2.

@Ma27
Copy link
Member

Ma27 commented Nov 9, 2018

First of all, thanks a lot for the additional effort! I'll look into it in further detail later.

Regarding nix-review (can be used by simply running nix run nixpkgs.nix-review), it's fairly simple: nix-review rev $rev_to_test :)

@Ma27
Copy link
Member

Ma27 commented Nov 10, 2018

So, I had a second look at your PR. As far as I can tell the main intention is still to bump pgcli to 2.0, right? In this case I'd default prompt_toolkit to v1 for now and wait until all applications moved (it seems as there's a bit to do when performing the upgrade in a Python application) and update the expression later. Actually doing the upgrade for prompt_tools may take some extra-effort. With this idea you'd only have two (re)builds: the build of prompt_tools and the rebuild of pgcli.

Additionally, I observed that the sandboxed build of xonsh is broken on your branch as you enabled the testcases test_printname and test_sourcefile. They internally reference /usr/bin/env and seem to rely on an impure state of a system :)

@marsam
Copy link
Contributor Author

marsam commented Nov 10, 2018

Sound reasonable. I've set the default prompt_toolkit to v1, and tested xonsh, ptpython with sandboxed builds.
I'm keeping ec5ba05 and 27241fe in this PR, but if you want it I can create separate PRs for them

Ma27
Ma27 approved these changes Nov 10, 2018
View reviewed changes
Copy link
Member

@Ma27 Ma27 left a comment

Choose a reason for hiding this comment

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

I pushed onto your branch to disable remaining impure tests you seem to have missed (they are already disabled on master).

I checked the functionality of xonsh, ptpython and pgcli locally, all of them remained functional.
By default prompt_tools is still v1 ATM, so no further breakage is expected (confirmed by running nix-review locally). When all of the remaining applications are moved, v1 can be dropped entirely.

And thanks a lot for your effort of course 👍

@Ma27 Ma27 merged commit fbc02d6 into NixOS:master Nov 10, 2018
@marsam marsam deleted the feature/update-pgcli branch November 11, 2018 04:54
@costrouc costrouc mentioned this pull request Nov 12, 2018
Closed
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ma27 Ma27 Ma27 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
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@marsam @Ma27 @GrahamcOfBorg