Disallow executable bit on .nix files

[patka-123]

I came across a nixpkgs PR that removes the executable bit from some .nix files. This can be prevented by a check in CI.

Would this project (nixpkgs-vet) be appropriate for adding such a check, or would it fit better in a standalone loose action?

If we decide where it should go I'd like to take a stab at it.

[infinisil]

Yeah this would be a pretty good fit for such a check! In particular it would fit around here:

nixpkgs-vet/src/structure.rs

Lines 170 to 176 in 0d26da8

	let result = result.and(if !package_nix_path.exists() {
	to_validation(PackageErrorKind::PackageNixNonExistent)
	} else if package_nix_path.is_dir() {
	to_validation(PackageErrorKind::PackageNixDir)
	} else {
	Success(())
	});

I think for this case it might be fine to just add a simple additional check and error, but in theory this could break master when a PR introduces an executable file but already went through CI before this change. So whenever strictness of checks is increased, a ratchet check would be most ideal to prevent such situations.

If you do want to go for the full ratchet check, you'd have to extend ratchet::Package with a something like a RatchetState<Executable>, change check_package to return validation::Result<HashMap<String, RatchetState<Executable>>>, and then incorporate those RatchetState<Executable> values into the construction of ratchet::Package in eval.rs (or a refactoring to move the construction out of that file).

While this would be neat, no offense taken if you don't go for that, a non-ratchet check is fine too (and perhaps somebody from @NixOS/nixpkgs-vet would be interested in making it a ratchet one) :)

[patka-123]

Does it really break master though? A nix file with an executable bit falling through the cracks doesn't seem like the end of the world to me. I personally think it'd be wiser to spend our time doing other work instead of creating a ratchet that most likely will not happen and is also not a critical bug. I can always set a reminder to run ! find -name '*.nix' -executable in a year from now, to clean up

I'll play around a bit and see what I can come up with (I'll be slow though). We can then always decide to do it completely differently if desired.

[infinisil]

Does it really break master though? A nix file with an executable bit falling through the cracks doesn't seem like the end of the world to me.

Scenario:

• There's a PR that adds an executable file, CI is successful because there's no check preventing that just yet
• We implement a check to ensure there are no executables, but it only runs for PRs that get updated
• The PR gets merged without updating it. CI is still green, so nothing seems wrong with doing so, but it breaks master, because new any updated PR will fail on the new executable file

The premise of the ratchet checks is to avoid such a situation, by checking the base branch against the PR branch, only ensuring that new instances of a bad pattern aren't introduced, not that there is no such pattern at all.

Granted, it's hard to say exactly what the chance is of this occurring. The fact that there were like 10 executable files before makes be believe it's not too uncommon, but maybe not worth creating a ratchet check for. Ratchet checks become less useful the less instances of a problematic pattern there are, but it's hard to draw the line.

At some point I would like to have the code in a state where it's just as easy to add a new ratchet check as it is to add a strict check. We're not there just yet, but what pattern works should become more obvious as more ratchet checks are implemented.

Anyways, I'd be happy to get a PR even for the basic check as a start :)

[willbush]

FWIW checking executable bit on current nixpkgs master (still?) returns nothing:

find . -type f -name "*.nix" -executable -print

[infinisil]

@willbush They were removed somewhat recently, I guess nobody added any since: NixOS/nixpkgs#341771

[willbush]

@infinisil still think this should be a ratchet check?

[infinisil]

Not worth it for now imo, but hopefully at some point ratchet checks are as easy as non-ratchet ones :)