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

kotatogram-desktop: init at 1.1.5 #75210

Merged
merged 3 commits into from Jan 31, 2020
Merged

kotatogram-desktop: init at 1.1.5 #75210

merged 3 commits into from Jan 31, 2020

Conversation

ilya-fedin
Copy link
Contributor

@ilya-fedin ilya-fedin commented Dec 8, 2019 •
edited

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

@balsoft
Copy link
Member

balsoft commented Dec 8, 2019

@GrahamcOfBorg build kotatogram-desktop libtgvoip rlottie-tdesktop

@balsoft
Copy link
Member

balsoft commented Dec 8, 2019 •
edited

It's better to split this into three commits, because there are three new packages added. Add them in the order of dependency -> dependent.

@balsoft
Copy link
Member

balsoft commented Dec 8, 2019

Also, if you have a bit of spare time to maintain the packages, please add yourself as a maintainer of these packages.

@balsoft
Copy link
Member

balsoft commented Dec 8, 2019

cc @primeos @abbradar

@ilya-fedin ilya-fedin force-pushed the kotatogram-desktop branch 3 times, most recently from 115a799 to d5f0538 Compare December 9, 2019 01:07
primeos
primeos previously requested changes Dec 13, 2019
View reviewed changes
Copy link
Member

@primeos primeos left a comment

Choose a reason for hiding this comment

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

This adds a lot of patches and files like CMakeLists.inj. Can those be fetched from somewhere (e.g. the AUR)? AFAIK fetching patches is the preferred solution (keeps Nixpkgs cleaner, is easier to review, the source is obvious, etc.).

The rest seems ok from a quick look. Thanks for your contribution :)

pkgs/applications/networking/instant-messengers/telegram/kotatogram-desktop/default.nix Outdated Show resolved Hide resolved
pkgs/applications/networking/instant-messengers/telegram/kotatogram-desktop/default.nix Show resolved Hide resolved
@ilya-fedin
Copy link
Contributor Author

ilya-fedin commented Dec 15, 2019 •
edited

This adds a lot of patches and files like CMakeLists.inj. Can those be fetched from somewhere (e.g. the AUR)?

No, preston significantly changed the directory tree in tdesktop v1.9.1, on which kotatogram-desktop v1.1.1 is based. I had to rewrite the patches almost from scratch (but I found a lot of unnecessary changes, most likely these patches were taken from flatpak - these changes are really needed there).
Only no-gtk2.patch remained unchanged.

The good news is that most patches will go away after 1-2 tdesktop/kotatogram updates (If they accept my PRs during this time).

Moreover, preston migrates to cmake: https://github.com/telegramdesktop/tdesktop/tree/cmake

@ilya-fedin ilya-fedin changed the title kotatogram-desktop: init at 1.1.1 kotatogram-desktop: init at 1.1.3 Jan 21, 2020
@ilya-fedin
Copy link
Contributor Author

Updated to k1.1.3 based on v1.9.4

primeos
primeos reviewed Jan 21, 2020
View reviewed changes
Copy link
Member

@primeos primeos left a comment

Choose a reason for hiding this comment

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

Seems a lot better now with way fewer patches :)

However, there are still quite a few patches left and I'm not sure what our policy is in such cases (personally I try to avoid it, but of course this isn't always possible). Might be a good idea to cc some very active PR reviewers, as they have way more experience with such cases than I have (e.g. @jonringer and @worldofpeace might be good fits). Or to mention this PR on Discourse, e.g. in this thread: https://discourse.nixos.org/t/prs-ready-for-review-may-2019/3032/.

@primeos primeos dismissed their stale review January 21, 2020 11:51

A large portion of the custom patches where successfully dropped

@ilya-fedin
Copy link
Contributor Author

cc @jonringer @worldofpeace

jonringer
jonringer reviewed Jan 21, 2020
View reviewed changes
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.

I feel like a lot of these patches could be upstreamed, and are not specific to nix

@ilya-fedin
Copy link
Contributor Author

So I should remove them?

@jonringer
Copy link
Contributor

By upstreaming I mean, theses could be submitted to the package repos themselves. Unless it's a nix-specific patch, then they should stay here. But many of these do not seem to be specific to NixOS/nixpkgs

@ilya-fedin ilya-fedin changed the title kotatogram-desktop: init at 1.1.3 kotatogram-desktop: init at 1.1.4 Jan 24, 2020
@ilya-fedin
Copy link
Contributor Author

Updated to k1.1.4 based on v1.9.7 and dropped all the patches

primeos
primeos reviewed Jan 24, 2020
View reviewed changes
Copy link
Member

@primeos primeos left a comment •
edited

Choose a reason for hiding this comment

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

Looks pretty good to me now, added some recommendations but would like a second opinion, mainly regarding the naming in all-packages.nix / the additional two packages in general.

pkgs/top-level/all-packages.nix Outdated
@@ -21416,6 +21416,12 @@ in
'';
tdesktop = qt5.callPackage ../applications/networking/instant-messengers/telegram/tdesktop { };

kotatogram-desktop = qt5.callPackage ../applications/networking/instant-messengers/telegram/kotatogram-desktop { };
Copy link
Member

Choose a reason for hiding this comment

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

The entries in this file should be sorted (roughly) alphabetically within the category (see comment at the beginning of the file).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

pkgs/top-level/all-packages.nix Outdated
Comment on lines 21421 to 21423
libtgvoip = callPackage ../development/libraries/libtgvoip { };

rlottie-tdesktop = callPackage ../development/libraries/rlottie-tdesktop { };
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about the naming here as both packages are forks. Maybe tdesktop-libtgvoip and tdesktop-rlottie? Also it's a bit unfortunate that none of them provide stable releases. An alternative could e.g. be to move them into ../applications/networking/instant-messengers/telegram/ and don't add them to all-packages.nix (i.e. directly import them in kotatogram-desktop.

Any thoughts on this from other reviewers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

libtgvoip upstream is dead and tdesktop's libtgvoip is the new upstream de-facto
I can enable bundling of rlottie

pkgs/development/libraries/libtgvoip/default.nix Outdated
meta = {
description = "VoIP library for Telegram clients";
longDescription = ''
VoIP library for Telegram clients
Copy link
Member

Choose a reason for hiding this comment

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

I'd remove longDescription if it's the same as description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

pkgs/applications/networking/instant-messengers/telegram/kotatogram-desktop/default.nix
description = "Kotatogram – experimental Telegram Desktop fork";
longDescription = ''
Unofficial desktop client for the Telegram messenger, based on Telegram Desktop.
It contains some useful (or purely cosmetic) features, but they could be unstable. A detailed list is available here: https://kotatogram.github.io/changes
Copy link
Member

Choose a reason for hiding this comment

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

Optional: A line break would be nice here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@jonringer
Copy link
Contributor

@GrahamcOfBorg build kotatogram-desktop
@GrahamcOfBorg build libtgvoip
@GrahamcOfBorg build rlottie-tdesktop

@ilya-fedin ilya-fedin force-pushed the kotatogram-desktop branch 3 times, most recently from 72b8ec8 to 381d917 Compare January 24, 2020 23:34
@primeos primeos mentioned this pull request Jan 24, 2020
Merged
10 tasks
@ilya-fedin
Copy link
Contributor Author

@GrahamcOfBorg build kotatogram-desktop libtgvoip

@ilya-fedin ilya-fedin changed the title kotatogram-desktop: init at 1.1.4 kotatogram-desktop: init at 1.1.5 Jan 25, 2020
@ilya-fedin
Copy link
Contributor Author

updated to k1.1.5 based on v1.9.8

jonringer
jonringer approved these changes Jan 30, 2020
View reviewed changes
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.

diff LGTM
application launches fine

[5 built, 88 copied (131.9 MiB), 24.3 MiB DL]
https://github.com/NixOS/nixpkgs/pull/75210
2 package built:
kotatogram-desktop libtgvoip

@jonringer
Copy link
Contributor

@GrahamcOfBorg build kotatogram-desktop libtgvoip

@jonringer
Copy link
Contributor

aarch failure unrelated

@jonringer jonringer merged commit 34890e4 into NixOS:master Jan 31, 2020
@ilya-fedin ilya-fedin deleted the kotatogram-desktop branch March 15, 2022 06:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@primeos primeos primeos left review comments

@worldofpeace worldofpeace worldofpeace left review comments

@jonringer jonringer jonringer approved these changes

@balsoft balsoft balsoft approved these changes

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 11.by: package-maintainer 12. first-time contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@ilya-fedin @balsoft @jonringer @primeos @worldofpeace @Janik-Haag