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

kitty: 0.18.1 → 0.18.2 #94240

Merged
merged 1 commit into from Jul 31, 2020
Merged

kitty: 0.18.1 → 0.18.2 #94240

merged 1 commit into from Jul 31, 2020

Conversation

sternenseemann
Copy link
Member

@sternenseemann sternenseemann commented Jul 30, 2020 •
edited

Motivation for this change

https://github.com/kovidgoyal/kitty/releases/tag/v0.18.2

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.

@ofborg ofborg bot requested review from Ma27, Luflosi and tex July 30, 2020 15:17
@sikmir
Copy link
Member

sikmir commented Jul 30, 2020

Result of nixpkgs-review pr 94240 on darwin 1

1 package failed to build:
- kitty 
builder for '/nix/store/cc6c9qb5l1d4frav2cv80cjhv1ir3zrm-kitty-0.18.2.drv' failed with exit code 1; last 10 log lines:
      cache[rtype].remove(name)
  KeyError: '/mp-vkbse8fx'
  Traceback (most recent call last):
    File "/nix/store/rf859gichqk3rvxw7xnlavpc8zalsk62-python3-3.8.3/lib/python3.8/multiprocessing/resource_tracker.py", line 201, in main
      cache[rtype].remove(name)
  KeyError: '/mp-onh27hyr'
  Traceback (most recent call last):
    File "/nix/store/rf859gichqk3rvxw7xnlavpc8zalsk62-python3-3.8.3/lib/python3.8/multiprocessing/resource_tracker.py", line 201, in main
      cache[rtype].remove(name)
  KeyError: '/mp-bkf0meiz'
cannot build derivation '/nix/store/51cgkdi3qv0j1127mwvxwjmxrp2bq82b-env.drv': 1 dependencies couldn't be built

@Mic92
Copy link
Member

Mic92 commented Jul 30, 2020

Result of nixpkgs-review pr 94240 on darwin 1
1 package failed to build:

  • kitty
builder for '/nix/store/cc6c9qb5l1d4frav2cv80cjhv1ir3zrm-kitty-0.18.2.drv' failed with exit code 1; last 10 log lines:
      cache[rtype].remove(name)
  KeyError: '/mp-vkbse8fx'
  Traceback (most recent call last):
    File "/nix/store/rf859gichqk3rvxw7xnlavpc8zalsk62-python3-3.8.3/lib/python3.8/multiprocessing/resource_tracker.py", line 201, in main
      cache[rtype].remove(name)
  KeyError: '/mp-onh27hyr'
  Traceback (most recent call last):
    File "/nix/store/rf859gichqk3rvxw7xnlavpc8zalsk62-python3-3.8.3/lib/python3.8/multiprocessing/resource_tracker.py", line 201, in main
      cache[rtype].remove(name)
  KeyError: '/mp-bkf0meiz'
cannot build derivation '/nix/store/51cgkdi3qv0j1127mwvxwjmxrp2bq82b-env.drv': 1 dependencies couldn't be built

Can you do a nix log /nix/store/cc6c9qb5l1d4frav2cv80cjhv1ir3zrm-kitty-0.18.2.drv ?

@sternenseemann
Copy link
Member Author

Oh great, another cryptic error message related to test_multiprocessing_spawn (?), I'll see whether I can figure this out.

@Mic92
Copy link
Member

Mic92 commented Jul 30, 2020

@GrahamcOfBorg build kitty

@Mic92
Copy link
Member

Mic92 commented Jul 30, 2020

We can also get the full log from ofborg...

@Mic92
Copy link
Member

Mic92 commented Jul 30, 2020

I would say just test_multiprocessing_spawn on macOS for now.

@sternenseemann
kitty: 0.18.1 → 0.18.2
c8f1df0 
For the newly introduced test_multiprocessing_spawn, we need to set
PATH to include the freshly built kitty binary, since it uses
kitty_exe() to determine the path to the binary which it uses to
spawn worker processes.
@sternenseemann
Copy link
Member Author

Fixed in c8f1df0.

@Luflosi
Copy link
Contributor

Luflosi commented Jul 30, 2020

Your commit was very helpful. I think I found a good way to do this in kitty so we don't need to change the PATH. I'll report back once I've made a PR upstream, then we can apply that as a patch.

@sternenseemann
Copy link
Member Author

It's only really a problem in the tests for nixpkgs, right? kitty is wrapped to include itself in PATH anyways normally.

@Luflosi
Copy link
Contributor

Luflosi commented Jul 30, 2020

Well, this problem could also happen in other environments when running the tests with kitty not in the path or in kitty/launcher, see kovidgoyal/kitty#2881 for example. For nixpkgs as well it's only a problem when running the tests as far as I know.
I made a PR in kovidgoyal/kitty#2885, which I expect to be reviewed and hopefully merged in a couple hours.

@Mic92
Copy link
Member

Mic92 commented Jul 31, 2020

@GrahamcOfBorg build kitty

@Mic92
Copy link
Member

Mic92 commented Jul 31, 2020

Result of nixpkgs-review pr 94240 1

1 package built:
- kitty

@Mic92 Mic92 merged commit b95c871 into NixOS:master Jul 31, 2020
@sternenseemann sternenseemann deleted the kitty-0.18.2 branch July 31, 2020 08:40
@Luflosi
Copy link
Contributor

Luflosi commented Jul 31, 2020

The solution upstream recommends is to build the launcher before running the tests, see kovidgoyal/kitty#2885. I think I'll do this when I update kitty the next time.

@Mic92
Copy link
Member

Mic92 commented Jul 31, 2020

There is an installCheckPhase for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ma27 Ma27 Awaiting requested review from Ma27

@Luflosi Luflosi Awaiting requested review from Luflosi

@tex tex Awaiting requested review from tex

Assignees
No one assigned
Labels
10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@sternenseemann @sikmir @Mic92 @Luflosi