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

dracula-theme: 1.3.0 -> 2.0 and rename ant-dracula to dracula-theme #99704

Merged
merged 2 commits into from Oct 11, 2020
Merged

dracula-theme: 1.3.0 -> 2.0 and rename ant-dracula to dracula-theme #99704

merged 2 commits into from Oct 11, 2020

Conversation

Vonfry
Copy link
Member

@Vonfry Vonfry commented Oct 6, 2020

Motivation for this change

Update package and rename it due to the upstream repo has been renamed.

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.

cc: @alexarice
fixes: #97655

alexarice
alexarice suggested changes Oct 6, 2020
View reviewed changes
Copy link
Contributor

@alexarice alexarice left a comment

Choose a reason for hiding this comment

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

nix-review passes. Have left some comments

maintainers/maintainer-list.nix
@@ -9088,6 +9088,12 @@
githubId = 508305;
name = "Jaroslavas Pocepko";
};
vonfry = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the change to maintainer-list.nix be in a separate commit titled "maintainers: add vonfry"

pkgs/data/themes/dracula-theme/default.nix Outdated
};

propagatedUserEnvPkgs = [
# gtk-engine-murrine
Copy link
Contributor

Choose a reason for hiding this comment

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

If this isn't needed this shouldn't be here

Copy link
Contributor

Choose a reason for hiding this comment

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

Think you've already removed this

pkgs/top-level/all-packages.nix
@@ -18767,7 +18767,7 @@ in

ant-bloody-theme = callPackage ../data/themes/ant-theme/ant-bloody.nix { };

ant-dracula-theme = callPackage ../data/themes/ant-theme/ant-dracula.nix { };
dracula-theme = callPackage ../data/themes/dracula-theme { };
Copy link
Contributor

Choose a reason for hiding this comment

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

This will break existing configurations. Maybe there should be an error thrown when using the old package that tells you to use the new one and to change the theme name in your config

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, typically you will log a warning for at least a couple of months.

Example:

nixpkgs/lib/debug.nix

Line 201 in 8b33d57

traceXMLVal = x:

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the alias should add an appropriate warning

@ofborg ofborg bot requested a review from alexarice October 6, 2020 07:58
@Vonfry Vonfry force-pushed the update/dracula branch 3 times, most recently from 8774780 to 710a9b7 Compare October 6, 2020 08:12
alexarice
alexarice suggested changes Oct 6, 2020
View reviewed changes
Copy link
Contributor

@alexarice alexarice left a comment

Choose a reason for hiding this comment

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

Is this the sort of change that should have a release note?

pkgs/top-level/aliases.nix Outdated
@@ -738,4 +738,7 @@ mapAliases ({
/* Cleanup before 21.03 */
riot-desktop = throw "riot-desktop is now element-desktop!";
riot-web = throw "riot-web is now element-web";

ant-dracula-theme = throw "ant-dracula-theme is now dracula-theme";
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this mention that the theme name has changed. For example I will have to change my config here

alexarice
alexarice approved these changes Oct 6, 2020
View reviewed changes
Copy link
Contributor

@alexarice alexarice left a comment

Choose a reason for hiding this comment

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

Looks good to me, Thanks for doing this fix, I wasn't even aware there had been an update.
I'll look into release notes, but other than that nix-review passes and looks good to go

@ofborg ofborg bot requested a review from alexarice October 6, 2020 08:23
alexarice
alexarice reviewed Oct 6, 2020
View reviewed changes
pkgs/data/themes/dracula-theme/default.nix Outdated
themeName = "Dracula";
version = "2.0";
in
stdenv.mkDerivation rec {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
stdenv.mkDerivation rec {
stdenv.mkDerivation {

Sorry, small change I missed

@ofborg ofborg bot requested a review from alexarice October 6, 2020 08:28
alexarice
alexarice approved these changes Oct 6, 2020
View reviewed changes
Copy link
Contributor

@alexarice alexarice left a comment

Choose a reason for hiding this comment

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

Asked on irc and I don't think there's any need for release notes so should be good as is

@ofborg ofborg bot requested a review from alexarice October 6, 2020 08:51
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/244

romildo
romildo reviewed Oct 11, 2020
View reviewed changes
pkgs/data/themes/ant-theme/ant-dracula.nix

propagatedUserEnvPkgs = [
gtk-engine-murrine
];
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is gtk-engine-murrine removed? It seems that the gtk2 theme still depends on this engine.

Copy link
Member Author

@Vonfry Vonfry Oct 11, 2020 •
edited

Choose a reason for hiding this comment

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

Sorry, I checked the source again. It uses this in gnome-terminal, which I don't use. I have added it back.

@romildo romildo merged commit e2064ae into NixOS:master Oct 11, 2020
@Vonfry Vonfry deleted the update/dracula branch October 12, 2020 01:09
@pjones pjones mentioned this pull request Nov 17, 2020
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevincox kevincox kevincox left review comments

@romildo romildo romildo left review comments

@alexarice alexarice Awaiting requested review from alexarice

Assignees
No one assigned
Labels
8.has: clean-up 8.has: package (new) 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.

Rename ant-dracula-theme → gtk-dracula-theme
5 participants
@Vonfry @nixos-discourse @kevincox @romildo @alexarice