goverlay: 0.7.1 -> 1.2#368202
Conversation
SigmaSquadron
left a comment
There was a problem hiding this comment.
Welcome to Nixpkgs! Please review the Contribution Guidelines for more information about how to format your commits. Notably, their titles should follow the packagename: oldversion -> newversion template. Ideally, the removal of find-xdg-data-files and the alphabetical sorting should also be separate commits, following the packagename: change description template.
There was a problem hiding this comment.
For consistency with other packages, please put lib as the first attribute in the function call, followed by stdenv and fetchFromGitHub.
There was a problem hiding this comment.
The sorting I can split out, I'm not sure how useful the removal of find-xdg-data-files is on its own. It was required with 0.7.1 but isn't with 1.2. Unless you're referring of the deletion of find-xdg-data-files.patch, than I can easily separate.
3566735 to
24f8d26
Compare
|
Looks good to me! The commits have been named appropriately and the diff looks alright. I'll wait until the other PR is merged before running a build.
|
24f8d26 to
cae7e81
Compare
|
|
Thanks for the update @Chais!! Just for reference, I was originally getting a build error: This was one of the reasons why I removed myself as maintainer on goverlay, but finally tracked the issue down to a bug in lazarus: https://bugs.gentoo.org/937421. I just had this option set in my NixOS config: nix.daemonCPUSchedPolicy = "idle"Temporarily removing this allows me to build the package again. |
kira-bruneau
left a comment
There was a problem hiding this comment.
Oh wait, I missed that CI is failing on formatting right now. Other than that though, this looks good to me!
@kira-bruneau I saw the formatting check failing but was unsure if it's something that should be remedied, given that |
Oh yep it should be remedied here. It's not complete yet, but there's been a big push to have everything formatted with nixfmt. There are still some files that haven't been formatted yet, but that's mainly to avoid merge conflicts. See #322537 |
|
|
024ec23 to
34e62a3
Compare
|
Oh I just ran it locally and pushed the diff. It looks like the issue was just an extra empty line. |
Oh, that's interesting. Yours arranged the arrays in one element per line again. What was it originally complaining about then? |
|
Oh it was just the extra blank line above |
0.7.1to1.2lazarus-qt6andlibqt6pasThings done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.