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

kitty: 0.17.4 -> 0.18.0 #91189

Merged
merged 1 commit into from Jun 21, 2020
Merged

kitty: 0.17.4 -> 0.18.0 #91189

merged 1 commit into from Jun 21, 2020

Conversation

Luflosi
Copy link
Contributor

@Luflosi Luflosi commented Jun 20, 2020

I submitted a patch to kitty, so that the library paths can now be passed as arguments to the build system, eliminating the need for patching. This should reduce the required maintenance effort in the future.
I removed an outdated comment.
I added a link to the changelog. Where is this link exposed to the user?
I added dontConfigure = true; to skip the configure phase since kitty has no configure script.
I now execute the test suite of kitty. Is the doCheck = true; attribute needed? It seems to work fine without but I added it anyways.
I also removed the dependency which.
Please check that the attributes are ordered in the preferred way.

Motivation for this change

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

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 tex and Ma27 June 20, 2020 20:06
@tex
Copy link
Contributor

tex commented Jun 20, 2020

Looks good to me. Can you squash commits into just one commit?

teto
teto approved these changes Jun 21, 2020
View reviewed changes
Copy link
Member

@teto teto left a comment •
edited

Choose a reason for hiding this comment

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

passed nixpkgs-review and binary worked fine. Maintainers can squash. Thanks for making kitty easier to maintain.

@Luflosi
kitty: 0.17.4 -> 0.18.0
f0e519f 
https://github.com/kovidgoyal/kitty/releases/tag/v0.18.0

I submitted a patch to kitty, so that the library paths can now be passed as arguments to the build system, eliminating the need for patching. This should reduce the required maintenance effort in the future.

This commit removes the dependency on `which`, which is no longer necessary since 5e4fe58. It was introduced in 481e61f.

The configure phase is now ignored because kitty has no configure script.

The kitty tests are now run after compilation.

The zsh completions no longer need to be invoked with `source` since 38eb737, so remove the outdated comment.

There is now a link to the changelog.
@Luflosi
Copy link
Contributor Author

Luflosi commented Jun 21, 2020

I put all the changes into one commit and rebased onto the latest master.

@teto teto merged commit f96feee into NixOS:master Jun 21, 2020
@ofborg ofborg bot requested a review from tex June 21, 2020 14:57
@Luflosi Luflosi deleted the update/kitty branch June 21, 2020 14:57
@Luflosi
Copy link
Contributor Author

Luflosi commented Jun 21, 2020

Thanks for merging ❤️.

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

@teto teto teto approved these changes

@Ma27 Ma27 Awaiting requested review from Ma27

@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 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Luflosi @tex @teto