You don't need pname if you're only using it to set the name.

Its more of a style choice, I prefer to have separate attributes for name and version. But I can understand that inconsistency might be a negative here. Do you want me to remove pname?

The standard way of doing this is not to define pname unless it's actively used for something.

please add runHook preConfigure and runHook postConfigure.

Isn't that only needed when preConfigure or postConfigure are actually used? Hooks don't use those right?

If someone needs to override something in a package, that can be done easily through the pre/post hooks. If you don't run them when you override a phase, they cannot be used.

Alright that makes sense. Is there a reason configurePhase etc. are not defined somewhat like this:

configurePhase() { 
	runHook preConfigure
	if [[ -n "$configurePhase" ]]; then
		eval "$configurePhase";
	else
		# what the default configure phase does
	fi
	runHook postConfigure
}

That would remove this necessary boiler plate and would remove confusion about how to execut phases when using `nix-shell.

I'm sure there's a reason - I just don't know what it is! Your suggestion makes perfect sense to me but @vcunat and/or @Ericson2314 might be able to shed some light on the current state of affairs.

I'm not sure. It's probably just because of history; I suspect it comes from beginnings of nixpkgs. In time it turned out you usually only want to override the inside and not pre/post, and now it's much more difficult to change such basic things.

runHook preInstall and runHook postInstall

you can use install directly instead of mkdir and cp

Instead of a comment, you can pass name = "do_not_hardcode_paths.patch to fetchpatch which also makes it easier to work with.

I added names (but also kept the comments, as they offer more freedom for explanation than a filename).

Documentation is always good!

Please don't use placeholder in nixpkgs. We need to stay compatible with people using Nix 1.11 for a while. I'm not sure why but this single instance apparently breaks nix-env -i nix-repl, as reported on IRC. This should be fixed ASAP.

Fixed in bae15c8

Sorry, I didn't know we're still maintaining compatibility. Is that documented somewhere? How long?

No idea for how long, but ther is a fil sthat specifies the minimum required nix version: https://github.com/NixOS/nixpkgs/blob/master/.version

Edit: whoops, meant https://github.com/NixOS/nixpkgs/blob/master/lib/minver.nix

Yeah, there's no date for it AFAIK. I personally think we shouldn't until 18.09 unless there's a good reason not to, while placeholders are nice it's generally pretty straightforward to work around the recursion in other ways.