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

gruvbox-dark-gtk: init at 1.0.1, gruvbox-dark-icons-gtk: init at 1.0.0 #109585

Merged
merged 3 commits into from Feb 4, 2021
Merged

gruvbox-dark-gtk: init at 1.0.1, gruvbox-dark-icons-gtk: init at 1.0.0 #109585

merged 3 commits into from Feb 4, 2021

Conversation

NomisIV
Copy link
Contributor

@NomisIV NomisIV commented Jan 16, 2021

Motivation for this change

I use these packages locally, and wanted them to become official. I have got them working properly, but the icon preview in lxappearance does not work. I don't know if it's a problem with the packaging, or the project. Icons seem to work nonetheless.

Please bear in mind that I'm new to NixOS, and contributing generally, so I might have missed something

Here are the git repositories for the projects:
https://github.com/jmattheis/gruvbox-dark-gtk
https://github.com/jmattheis/gruvbox-dark-icons-gtk

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"(completed with No diff detected, stopping review...)
  • Tested execution of all binary files (usually in ./result/bin/) (The packages successfully build, and the themes can be applied using lxappearance)
  • 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.

pkgs/data/icons/gruvbox-dark-icons-gtk/default.nix Outdated Show resolved Hide resolved
pkgs/data/icons/gruvbox-dark-icons-gtk/default.nix Outdated Show resolved Hide resolved
pkgs/data/icons/gruvbox-dark-icons-gtk/default.nix Outdated Show resolved Hide resolved
pkgs/data/themes/gruvbox-dark-gtk/default.nix Outdated Show resolved Hide resolved
pkgs/data/themes/gruvbox-dark-gtk/default.nix Outdated Show resolved Hide resolved
@SuperSandro2000 SuperSandro2000 changed the title Add package gruvbox dark gtk theme and icons gruvbox-dark-gtk: init at 1.0.1, gruvbox-dark-icons-gtk: init at 1.0.0 Jan 17, 2021
@SuperSandro2000
Copy link
Member

I just renamed the title to more easily reflect the commits that are in here.

Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

Please squash the package related commits together.

@NomisIV
Copy link
Contributor Author

NomisIV commented Jan 18, 2021

Well, that didn't work like intended...
I tried doing a git rebase -i HEAD~9, and changed the relevant picks to squash instead, but I'm unsure as to what that actually did. What am I supposed to do? Please excuse my inexperience

@SuperSandro2000
Copy link
Member

The right command is head git rebase -i @~9. I am not sure what you did now but it is not the right thing and merging master into a PR is a big no go.

@cole-h
Copy link
Member

cole-h commented Jan 18, 2021

(FWIW, HEAD~9 works fine.)

You first do the git rebase command you used. Then, you choose the commits you want to squash, but you must order them to come DIRECTLY AFTER the commit you want to squash them into. e.g.

pick asdf1 commit 1
pick asdf2 commit 2
pick asdf3 commit 3, intended to become part of commit 1
pick asdf4 commit 4, intended to become part of commit 2
pick asdf5 commit 5, intended to become part of commit 1
pick asdf6 commit 6, intended to become part of commit 1
pick asdf7 commit 7, intended to become part of commit 2

would become

pick asdf1 commit 1
squash asdf3 commit 3, intended to become part of commit 1
squash asdf5 commit 5, intended to become part of commit 1
squash asdf6 commit 6, intended to become part of commit 1
pick asdf2 commit 2
squash asdf4 commit 4, intended to become part of commit 2
squash asdf7 commit 7, intended to become part of commit 2

I usually use fixup instead of squash, since I don't care about the commit message most times.

@SuperSandro2000
Copy link
Member

I usually use fixup instead of squash, since I don't care about the commit message most times.

Actually this is what I do all the time, too but it is still called squashing.

@NomisIV
Copy link
Contributor Author

NomisIV commented Jan 19, 2021

but you must order them to come DIRECTLY AFTER the commit you want to squash them into

That's exactly what I did, so I don't know what I did wrong.

I am not sure what you did now but it is not the right thing and merging master into a PR is a big no go.

Are you referring to this pull request 971f2a1? If yes, I don't know how to avoid doing that. After the rebase git wanted me to pull changes for some reason, and since I had local unpushed commits, the merge emerged.

Is there a simple way to rescue this disaster? I'm thinking of attempting something like this, but figured I should probably ask someone who knows what to do before I screw things up more.

Another thing I could try is to start from scratch and redo the changes in the PR, but I'd like to learn how to do it properly too.

I learn from my mistakes ;)

@SuperSandro2000
Copy link
Member

You can take the changes from this PR, delete the branch, create it under the new name and commit the changes into 3 commits and then force push.

@cole-h
Copy link
Member

cole-h commented Jan 19, 2021

Instead of creating a merge commit, you usually want to git pull --rebase which will automatically rebase your local changes on top of wherever you're pulling from. If there are conflicts, you'll be able to fix them and git rebase --continue until your changes are "compatible" with wherever you pulled from.

@ofborg eval

@NomisIV
Copy link
Contributor Author

NomisIV commented Jan 27, 2021

I was able to fix the problem by rebasing a second time, and using git push --force to push the changes. I didn't use the force flag the first time, and that's probably why the commits didn't squash.

@NomisIV
Copy link
Contributor Author

NomisIV commented Jan 27, 2021

I have now added @romildo's suggested changes, some even in both files (like removing unneeded files, and not copying ${src} directly)

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.

kdeFrameworks doesn't appear in nix repl ., which is probably why eval failed. I changed kdeFrameworks to plasma5Packages, reordered the commits, and force-pushed, which fixed the eval error.

Now, diff LGTM. GitHub ID also checks out. Thanks :)

@cole-h cole-h merged commit 7163640 into NixOS:master Feb 4, 2021
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