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

xonsh: 0.8.3 -> 0.8.12 #58824

Merged
merged 1 commit into from Apr 14, 2019
Merged

xonsh: 0.8.3 -> 0.8.12 #58824

merged 1 commit into from Apr 14, 2019

Conversation

Meptl
Copy link
Contributor

@Meptl Meptl commented Apr 2, 2019 •
edited

  • Expanded unit tests.
  • Added pytest-rerunfailures due to flaky tests.
Motivation for this change

Failing hydra build.
https://hydra.nixos.org/build/91523333
/cc #56826

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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Tests

Note that to build this with a local master you need a small edit to pytest-rerunfailures. Add pytest to checkInputs. See part of #58710. <- PR released

  • nix-build -A xonsh
  • nix-shell -I nixpkgs=/path/to/nixpkgs xonsh --pure -p xonsh

Meptl
Meptl commented Apr 3, 2019
View reviewed changes
pkgs/shells/xonsh/default.nix
};

LC_ALL = "en_US.UTF-8";
postPatch = ''
sed -ie "s|/bin/ls|${coreutils}/bin/ls|" tests/test_execer.py
sed -ie 's|/usr/bin/env|${coreutils}/bin/env|' scripts/xon.sh
sed -ie "s|SHELL=xonsh|SHELL=$out/bin/xonsh|" tests/test_integrations.py
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm using $out/bin/xonsh because (if I understand correctly) for python projects the checkPhase == installCheckPhase, so we want to reference the "installed" binary.

Meptl
Meptl commented Apr 3, 2019
View reviewed changes
pkgs/shells/xonsh/default.nix
-k 'not test_man_completion and not test_indir and not test_xonsh_party and not test_foreign_bash_data and not test_script and not test_single_command_no_windows and not test_redirect_out_to_file and not test_sourcefile and not test_printname and not test_printfile'
HOME=$TMPDIR pytest -k 'not test_repath_backslash and not test_os and not test_man_completion and not test_builtins and not test_main and not test_ptk_highlight'
HOME=$TMPDIR pytest -k 'test_builtins or test_main' --reruns 5
HOME=$TMPDIR pytest -k 'test_ptk_highlight'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Running this one separately is something that xonsh does as well. See xonsh/run-tests.xsh.

@Ma27
Copy link
Member

Ma27 commented Apr 6, 2019

@GrahamcOfBorg build xonsh

@Meptl Meptl requested a review from FRidh as a code owner April 6, 2019 14:03
Ma27
Ma27 approved these changes Apr 6, 2019
View reviewed changes
Copy link
Member

@Ma27 Ma27 left a comment •
edited

Choose a reason for hiding this comment

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

The build broke locally due to python.pkgs.pytest-rerunfailures being broken, I just pushed a fix. As soon as ofBorg finished successfully, I'd merge and backport 👍

Just seen that it's on master already, but the PR contains an old state.

@Ma27
Copy link
Member

Ma27 commented Apr 6, 2019

@GrahamcOfBorg build xonsh

@Ma27
Copy link
Member

Ma27 commented Apr 6, 2019

Just a short update: I removed my commit again as I missed that pytest-rerunfailures is already fixed on master. I'm a bit hesitant now to merge this and probably leave the darwin build broken. I'll see if I can bootstrap a testing environment for this locally and report back.

@Meptl
Copy link
Contributor Author

Meptl commented Apr 7, 2019

I don't have a mac myself, but I was able to run some tests through travis. https://travis-ci.org/Meptl/nixpkgs/builds/516748079 (darwin-test branch). Copied the nix-build command from OfBorg README. I don't think the most recent travis osx image is nix-ready. Still investigating the travis environment to see how I might reproduce.

@Ma27
Copy link
Member

Ma27 commented Apr 7, 2019

I don't have a mac myself, but I was able to run some tests through travis

Thanks a lot for doing this! I don't own a mac either, I planned to test this in a virtualbox vm with osx today, but you were faster regarding a macos test 😅

The only thing I'm not sure about atm is whether or not to keep the tests in darwin or to disable those with doCheck = !stdenv.isDarwin.

@grahamc would you mind having a look at the ofborg errors for darwin? Do you think that this is related to ofborg's testing environment or should we entirely disable the tests on darwin for now? :)

@timokau
Copy link
Member

timokau commented Apr 13, 2019

Since xonsh is currently broken on master, should we just exclude darwin for now? If there is interest from a mac user, they are in a much better position to fix it.

@Ma27
Copy link
Member

Ma27 commented Apr 13, 2019

That makes sense, yes 👍

This should be backported as well then, IIRC the package is broken on release-19.03 as well.

@Meptl
Copy link
Contributor Author

Meptl commented Apr 13, 2019

Rebased off latest master, added the doCheck. I force pushed twice because I used the wrong email as the commit author. Kicked off OSX tests for every image available on travis: https://travis-ci.org/Meptl/nixpkgs/builds/519768510 (darwin-test2 branch)

@timokau
Copy link
Member

timokau commented Apr 14, 2019

@GrahamcOfBorg build xonsh

@timokau timokau merged commit aa4af91 into NixOS:master Apr 14, 2019
@Ma27
Copy link
Member

Ma27 commented Apr 14, 2019

@timokau can you backport to release-19.03, please? :)

timokau pushed a commit to timokau/nixpkgs that referenced this pull request Apr 14, 2019
@Meptl @timokau
xonsh: 0.8.3 -> 0.8.12 (NixOS#58824)
fde7a9c 
(cherry picked from commit aa4af91)
@timokau
Copy link
Member

timokau commented Apr 14, 2019

#59509

@timokau timokau added the 8.has: port to stable A PR already has a backport to the stable release. label Apr 14, 2019
timokau added a commit that referenced this pull request Apr 14, 2019
@timokau
xonsh: 0.8.3 -> 0.8.12 (#58824) (#59509)
b8debac 
(cherry picked from commit aa4af91)
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

@vrthra vrthra Awaiting requested review from vrthra

@garbas garbas Awaiting requested review from garbas

@FRidh FRidh Awaiting requested review from FRidh

Assignees
No one assigned
Labels
8.has: package (update) 8.has: port to stable A PR already has a backport to the stable release. 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
5 participants
@Meptl @Ma27 @timokau @infinisil @GrahamcOfBorg