What did I do wrong in this PR? (My first nixpkgs PR)

Development
1

I created a PR some time last year to fix an issue that has been bugging me for a while, but I found the process confusing and intimidating.

I checked off some boxes, but the whole time I worried that seeing a not-fully-checked checklist on a PR might make people ignore it. It’s not clear to me if it’s “do this before submitting” or if it’s “for the record this is how tested it is”. I ran nixpkgs-review but I have absolutely no idea what it did / what it’s supposed to do / if I used it right. Nobody has commented on the PR, so perhaps I’ve done it so wrong that people thought it was a draft still?

It looks like someone was assigned as a reviewer at some point but I never got any human interaction on the topic at all. I never received any emails or notifications from GitHub about the PR because nobody ever said anything on it. The best I got is noticing my laptop wouldn’t update, realising it’s because nixpkgs was on my branch, then thinking “oh right” and coming back to GitHub to check the PR, only to find out had been marked stale.

I really worry that I’ve just put several solid hours of effort into debugging and fixing something that bothered me, done the whole “be the change you want to see in the world” thing, and still hit an unspoken wall of “it’s just xprofile, nobody cares”.

And now it has (trivial) merge conflicts, but last time I tried to fix them I had trouble getting the test suite running at all and gave up in frustration, thinking, why bother fixing merge conflicts if they’re just going to happen again in another month’s time?

3 Likes
2

I found the process confusing and intimidating.

Sorry about that!

the whole time I worried that seeing a not-fully-checked checklist on a PR might make people ignore it. It’s not clear to me if it’s “do this before submitting” or if it’s “for the record this is how tested it is”.

It’s “for the record” - the comment above it says “Please check what applies. Note that these are not hard requirements but merely serve as information for reviewers.“. Do you have ideas on how to make that clearer?

I think you used it right (though in this case it didn’t really do anything useful: apparently your change only impacted packages that aren’t tested/testable with nixpkgs-review)

(I’m no fan of the stalebot either)

So, on to things you could do:

Specific to this PR: skimming the PR I saw “this re-executes the whole script“ and was like “Huh, so it doesn’t really fix the issue?”. Of course on a closer look the exec means that the ‘first run’ in this case doesn’t actually ‘really’ run the script, but wraps it in systemd-cat. Maybe this comment could somehow be clearer?

That’s very understandable and sadly the above is no guarantee - but starting this conversation might be a good start!

4 Likes
3

I agree about pinging the people who filed those issues.

Also, I usually ping the maintainer of the thing I’m modifying. Even if there is no designated maintainer, I look at the file history to see who authored the last changes to the file(s).

4

But still some PRs might take a long time to get merged. Using your own branch of Nixpkgs is kinda annoying. Especially if you have multiple open PRs.

For this I recommend using Nixtamal. It makes patching inputs easy. Let’s say you have the following manifest:

inputs {
	nixpkgs {
		archive {
			url "{{fresh_value}}/nixexprs.tar.xz"
		}
		hash algorithm=SHA-256
		fresh-cmd {
			$ curl -Ls --http2 --compressed -o "/dev/null" -w "%{url_effective}" "https://channels.nixos.org/nixos-unstable"
		}
	}

	// Other Inputs...
}

Now when you (or someone else for that matter) make a PR to Nixpkgs, and want to use the patch before it’s merged. You modify your manifest to become:

patches {
	// NOTE: Define Patch Here
	nixpkgs-pr "https://github.com/NixOS/nixpkgs/pull/432404.patch"
}

inputs {
	nixpkgs {
		archive {
			url "{{fresh_value}}/nixexprs.tar.xz"
		}
		hash algorithm=SHA-256
		fresh-cmd {
			$ curl -Ls --http2 --compressed -o "/dev/null" -w "%{url_effective}" "https://channels.nixos.org/nixos-unstable"
		}

		// NOTE: Use Patch Here
		patches nixpkgs-pr
	}

	// Other Inputs...
}

I used your PR for this example. Notice how I just appended .patch to the PR’s URL.

GitHub provides other ways to get those patches. So you can even test changes before opening a PR.

Now you are free to keep updating Nixpkgs from your selected channel without having to sync your fork. You can use multiple patches, from multiple people, without much hassle.

And it’s not limited to Nixpkgs, it can be used with any of your Nix inputs.

I contribute to Nixpkgs and other Nix-related projects I use frequently. Nixtamal made my life MUCH easier.

5

As for my two cents, if you are already using a flake, the more newb-friendly approach might be nixpkgs-patcher. In this case patches are just flake inputs with flake = false;. Then again I think I’d use nixtamal if I weren’t so deep in flakes/flake-parts already… :sweat_smile:

1 Like
6

Thanks for your work, @ScoreUnder! As the author of .profile (and .xprofile) sourced twice by display manager · Issue #188545 · NixOS/nixpkgs · GitHub, I am definitely interested, and would have reviewed your PR if I knew if existed. Posting a comment on the other issues would have let everyone subscribed know that someone’s working on it.

I will take a look at your PR later today.

5 Likes
7

Also, if your PR hasn’t seen any responses in a month or so, despite pinging relevant people, there is this thread: PRs in distress

The unfortunate reality is that nixpkgs gets so many PRs that you kinda have to nag your way to getting someone to review them.

If you aren’t a prolific maintainer and don’t start out guns blazing, your PR will fall through the cracks.

5 Likes