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

chatterino2: unstable-2019-05-11 -> 2.1.7 #79073

Closed
wants to merge 3 commits into from

Conversation

strager
Copy link
Contributor

@strager strager commented Feb 2, 2020

Upgrade chatterino2 to the latest release version.

This commit changes the name of the chatterino2 package. Before this commit, the package is named chatterino2-unstable. After this commit, the package is named chatterino2. If a user wants to upgrade, they must first uninstall chatterino2-unstable before installing chatterino2, else there will be a conflict:

$ nix-env -iA nixpkgs.chatterino2
installing 'chatterino2-2.1.7'
building '/nix/store/hxx0vriblvvlkskcnm4rk32d2ghlvkj5-user-environment.drv'...
error: packages '/nix/store/icgg81lkh1z17zbzw7ncrfqnkhlb39x9-chatterino2-2.1.7/Applications/chatterino.app/Contents/Info.plist' and '/nix/store/65wapk4pd5gj8dgvd245fzv4z799cnw8-chatterino2-unstable-2019-05-11/Applications/chatterino.app/Contents/Info.plist' have the same priority 5; use 'nix-env --set-flag priority NUMBER INSTALLED_PKGNAME' to change the priority of one of the conflicting packages (0 being the highest priority)
builder for '/nix/store/hxx0vriblvvlkskcnm4rk32d2ghlvkj5-user-environment.drv' failed with exit code 1
error: build of '/nix/store/hxx0vriblvvlkskcnm4rk32d2ghlvkj5-user-environment.drv' failed
$ nix-env -q | grep chatterino2
chatterino2-unstable-2019-05-11

$ # chatterino2-unstable conflicts with chatterino2; both can't be
$ # installed at the same time. Uninstall chatterino2-unstable, then
$ # try installing chatterino2 again:

$ nix-env -e chatterino2-unstable
uninstalling 'chatterino2-unstable-2019-05-11'
$ nix-env -iA nixpkgs.chatterino2
installing 'chatterino2-2.1.7'
$ nix-env -q | grep chatterino2
chatterino2-2.1.7
Motivation for this change
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.

@strager strager changed the title Chatterino2 chatterino2: unstable-2019-05-11 -> 2.1.7 Feb 2, 2020
@doronbehar
Copy link
Contributor

This commit changes the name of the chatterino2 package. Before this commit, the package is named chatterino2-unstable. After this commit, the package is named chatterino2. If a user wants to upgrade, they must first uninstall chatterino2-unstable before installing chatterino2, else there will be a conflict

That's totally reasonable. Unfortunately nix-env doesn't have a way to know reliably which packages need upgrades as it uses the name attribute to know if an update is needed and not the package's attribute.

@doronbehar

This comment has been minimized.

@doronbehar
Copy link
Contributor

Please fix the merge conflicts. Any idea why is this marked as Draft?

@strager
Copy link
Contributor Author

strager commented Feb 23, 2020

Any idea why is this marked as Draft?

This PR depends on #79067. #79067 should merge first, then we can merge #79073.

@strager strager marked this pull request as ready for review February 25, 2020 18:08
@strager
Copy link
Contributor Author

strager commented Feb 25, 2020

This PR is ready to be merged.

@cole-h
Copy link
Member

cole-h commented Feb 28, 2020

Any reason this one should be merged over #77994?

@strager
Copy link
Contributor Author

strager commented Feb 28, 2020

Any reason this one should be merged over #77994?

I didn't know #77994 exited when I created my PR. =X

It looks like #77994 has some merge conflicts (which should be easy to fix). My PR (#79073) can merge cleanly.

Copy link
Member

@cole-h cole-h left a comment

Choose a reason for hiding this comment

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

I'd like to ask that you format the file (insert newlines, etc) so that it is easier to look at. It doesn't really matter, but comparing the two PRs, I myself would merge the other one solely because it's easier on the eyes.

Do note that I don't have the power to enforce this; it's just personal preference. Feel free to ignore the suggestion.

meta = with lib; {
description = "A chat client for Twitch chat";
longDescription = ''
Chatterino is a chat client for Twitch chat. It aims to be an
improved/extended version of the Twitch web chat. Chatterino 2 is
the second installment of the Twitch chat client series
"Chatterino".
'';
'';
homepage = "https://github.com/fourtf/chatterino2";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
homepage = "https://github.com/fourtf/chatterino2";
homepage = "https://github.com/Chatterino/chatterino2";

fetchSubmodules = true;
};
nativeBuildInputs = [ qmake pkgconfig ];
nativeBuildInputs = [ qmake pkgconfig wrapQtAppsHook ];
patches = lib.optionals stdenv.isDarwin [ ./darwin-frameworks.patch ];
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment about why this is necessary, and why it can't be upstreamed?


mkDerivation rec {
pname = "chatterino2";
version = "unstable-2019-05-11";
version = "2.1.7";
src = fetchFromGitHub {
owner = "fourtf";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
owner = "fourtf";
owner = "Chatterino";

@strager
Copy link
Contributor Author

strager commented Feb 28, 2020

To clarify: the following commits have already been merged and shouldn't be part of this PR:

  • cdd1c21 chatterino2: fix install on macOS
  • 1221a6b chatterino2: fix: Could not find the Qt platform plugin "cocoa" in ""

This PR only introduces the following commits:

  • 369381e chatterino2: fix formatting of Nix code
  • c914ffc chatterino2: unstable-2019-05-11 -> 2.1.7

GitHub's UI is misleading about what commits are part of this PR. I tried to fix this PR, but GitHub gave me server errors...

@strager strager changed the base branch from master to staging February 28, 2020 20:13
@strager strager changed the base branch from staging to master February 28, 2020 20:13
@strager
Copy link
Contributor Author

strager commented Feb 28, 2020

I managed to fix this PR by changing the base to staging then back to master. On my screen GitHub now correctly shows only the two commits I intend to be part of this PR. Hopefully this clears up confusion.

@cole-h
Copy link
Member

cole-h commented Feb 28, 2020

You probably need to force-push.

Also, I think it would be better to just have the "fix formatting" commit squashed into the actual version bump. The only difference is two spaces...

Copy link
Member

@cole-h cole-h left a comment

Choose a reason for hiding this comment

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

Eval is failing for unrelated reasons (see: #81325), so rest assured it's not your fault :)

@strager
Copy link
Contributor Author

strager commented Feb 28, 2020

wrapQtAppsHook should be specified iff you cannot use the qt-specific deriver: "If you cannot use mkDerivation or mkDerivationWith above, include wrapQtAppsHook in nativeBuildInputs" (emphasis mine).

The docs say "if", not "iff".

This wasn't resolved

wrapQtAppsHook was discussed in PR #79067.

Regardless, the presence or absence of wrapQtAppsHook is unrelated to this PR (#79073). This PR is about upgrading chatterino2, not fixing style [1] or other issues in chatterino2's .nix file.

[1] The whitespace change isn't worth its own PR, so it came along for the ride here.

@cole-h
Copy link
Member

cole-h commented Feb 29, 2020

You're right. I just figured "since you're already here, it might be nice to make the file more readable." I overlooked your conversation about the presence of wrapQtAppsHook and now that I see where you're coming from, it makes sense. Sorry I became so confrontational and tried to force my opinion on you.


After my prevoius review comments get addressed -- leaving a comment why the patch is necessary and changing the repo owner from fourtf to Chatterino -- I can't wait to see this merged and drop my overlay :)

Upgrade chatterino2 to the latest release version.

This commit changes the name of the chatterino2 package. Before this
commit, the package is named chatterino2-unstable. After this commit,
the package is named chatterino2. If a user wants to upgrade, they must
first uninstall chatterino2-unstable before installing chatterino2, else
there will be a conflict:

    $ nix-env -iA nixpkgs.chatterino2
    installing 'chatterino2-2.1.7'
    building '/nix/store/hxx0vriblvvlkskcnm4rk32d2ghlvkj5-user-environment.drv'...
    error: packages '/nix/store/icgg81lkh1z17zbzw7ncrfqnkhlb39x9-chatterino2-2.1.7/Applications/chatterino.app/Contents/Info.plist' and '/nix/store/65wapk4pd5gj8dgvd245fzv4z799cnw8-chatterino2-unstable-2019-05-11/Applications/chatterino.app/Contents/Info.plist' have the same priority 5; use 'nix-env --set-flag priority NUMBER INSTALLED_PKGNAME' to change the priority of one of the conflicting packages (0 being the highest priority)
    builder for '/nix/store/hxx0vriblvvlkskcnm4rk32d2ghlvkj5-user-environment.drv' failed with exit code 1
    error: build of '/nix/store/hxx0vriblvvlkskcnm4rk32d2ghlvkj5-user-environment.drv' failed
    $ nix-env -q | grep chatterino2
    chatterino2-unstable-2019-05-11

    $ # chatterino2-unstable conflicts with chatterino2; both can't be
    $ # installed at the same time. Uninstall chatterino2-unstable, then
    $ # try installing chatterino2 again:

    $ nix-env -e chatterino2-unstable
    uninstalling 'chatterino2-unstable-2019-05-11'
    $ nix-env -iA nixpkgs.chatterino2
    installing 'chatterino2-2.1.7'
    $ nix-env -q | grep chatterino2
    chatterino2-2.1.7
On GitHub, chatterino2 moved from https://github.com/fourtf/chatterino2
to https://github.com/Chatterino/chatterino2. The old URL redirects to
the new URL, but update Nixpkgs' source URL and homepage to point to the
new URL to reduce confusion.
@strager
Copy link
Contributor Author

strager commented May 26, 2020

@cole-h I found that chatterino2's upstream had a patch for qtkeychain, so I used that patch. Also, I updated the GitHub URL as you requested.

Copy link
Member

@cole-h cole-h left a comment

Choose a reason for hiding this comment

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

LGTM, thanks. Connects to Twitch chat and lets me interact as expected.

[3 built, 0.0 MiB DL]
https://github.com/NixOS/nixpkgs/pull/79073
1 package built:
chatterino2

@@ -0,0 +1,27 @@
Fix build on macOS.
Copy link
Member

Choose a reason for hiding this comment

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

Slight nit: the comment about the patch should go above the patch in the derivation, not inside the patch itself. The first place people look (or at least, the first place I look) for the reasoning behind a patch is in the derivation itself.

@bbigras
Copy link
Contributor

bbigras commented Sep 12, 2020

superseded by a11a629

@bbigras bbigras closed this Sep 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants