Skip to content

Comments

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

Merged
romildo merged 2 commits intoNixOS:masterfrom
Vonfry:update/dracula
Oct 11, 2020
Merged

dracula-theme: 1.3.0 -> 2.0 and rename ant-dracula to dracula-theme#99704
romildo merged 2 commits intoNixOS:masterfrom
Vonfry:update/dracula

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 Outdated
Copy link
Contributor

@alexarice alexarice Oct 6, 2020

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
Copy link
Contributor

@alexarice alexarice Oct 6, 2020

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

@alexarice alexarice Oct 6, 2020

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 Outdated
Copy link
Contributor

@alexarice alexarice Oct 6, 2020

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

@kevincox kevincox Oct 11, 2020

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

@alexarice alexarice Oct 11, 2020

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 added 8.has: clean-up This PR removes packages or removes other cruft 8.has: package (new) This PR adds a new package labels Oct 6, 2020
@ofborg ofborg bot requested a review from alexarice October 6, 2020 07:58
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels Oct 6, 2020
@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
Copy link
Contributor

@alexarice alexarice Oct 6, 2020

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
Copy link
Contributor

@alexarice alexarice Oct 6, 2020

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

nixos-discourse commented Oct 11, 2020

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 Outdated
Copy link
Contributor

@romildo romildo Oct 11, 2020

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
Loading

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 This PR removes packages or removes other cruft 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes.

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