nixpkgs-check-by-name: Intermediate error representation refactor#261939
nixpkgs-check-by-name: Intermediate error representation refactor#261939infinisil merged 28 commits intoNixOS:masterfrom
nixpkgs-check-by-name: Intermediate error representation refactor#261939Conversation
ofborg
bot
added
10.rebuild-darwin: 1-10
Currently the tool prints problems right as it is checking the code without an intermediate error representation. However for various reasons it would be beneficial to have an intermediate error type: - It makes the code cleaner, having all errors in one place - It allows printing the error in different ways, e.g. for a future --json mode This commit prepares for an incremental refactoring for an intermediate error/problem representation. Most notable is that we want to be able to collect multiple errors/problems and not just exit on the first one. We introduce the type alias CheckResult and CheckError (later renamed to NixpkgsProblem), where CheckError allows collecting multiple CheckErrors using the utility function flatten_check_results (later renamed to check_result::sequence) The write_check_result function is only temporarily introduced to help refactoring, it's removed again in later commits.
infinisil
force-pushed
the
check-by-name-intermediate
branch
from
655f147 to
568840b
Compare
Isn't necessary anymore with the refactoring
And introduce a function for some smaller indentation
infinisil
force-pushed
the
check-by-name-intermediate
branch
from
568840b to
03c58ad
Compare
infinisil
changed the title
nixpkgs-check-by-name to have an intermediate error representationnixpkgs-check-by-name: Intermediate error representation refactor
whentze
left a comment
whentze
left a comment
There was a problem hiding this comment.
LGTM overall, but I left some comments.
| /// A type alias representing the result of a check, either: | ||
| /// - Err(anyhow::Error): A fatal failure, typically I/O errors. | ||
| /// Such failures are not caused by the files in Nixpkgs. | ||
| /// This hints at a bug in the code or a problem with the deployment. | ||
| /// - Ok(Left(Vec<NixpkgsProblem>)): A non-fatal problem with the Nixpkgs files. | ||
| /// Further checks can be run even with this result type. | ||
| /// Such problems can be fixed by changing the Nixpkgs files. | ||
| /// - Ok(Right(A)): A successful (potentially intermediate) result with an arbitrary value. | ||
| /// No fatal errors have occurred and no problems have been found with Nixpkgs. | ||
| pub type CheckResult<A> = anyhow::Result<Either<Vec<NixpkgsProblem>, A>>; |
There was a problem hiding this comment.
This works, but is a bit clumsy I feel. You have the Either that is documented nicely here but the meaning of left/right is lost at all the usage sites, and you have something that is ostensibly an error case in your Ok variant, which in turn begets the custom utility functions like map and ok below.
Potential alternative: Make a new Error type like
enum CheckError {
Fatal(anyhow::Error),
NonFatal(Vec<NixpkgsProblem>),
}then implement From<anyhow::Error> for it so you get to keep using ? ergonomically.
Your CheckResult would then just be Result<CheckError>, and you can use the regular map and Ok from vanilla Result.
There was a problem hiding this comment.
For others: I paired with @whentze today, where we concluded that this doesn't work out very well, in short, the Result type is made for early exits, but in our case Err(NonFatal(_)) should not exit early, which makes the code rather complicated.
Instead we ended up going for:
- A new type
Validation(leaning onto Haskell'sValidation, which inspired this in the first place) instead ofEither - Handling
Result's as they happen in the checks using?, so this complexity is not in theValidationfunctions
There are now somewhat ugly Ok(...)'s wrapping parts of the code, but it's not terrible imo :)
| impl NixpkgsProblem { | ||
| /// Create a `CheckResult<A>` from a single check problem | ||
| pub fn into_result<A>(self) -> CheckResult<A> { | ||
| Ok(Left(vec![self])) | ||
| } | ||
| } |
There was a problem hiding this comment.
Same here, if you use vanilla Result you can impl From<NixpkgsProblem> for CheckError and get to use ?.
| struct PackageContext<'a> { | ||
| /// The package directory relative to Nixpkgs, such as `pkgs/by-name/fo/foo` | ||
| relative_package_dir: &'a PathBuf, | ||
| /// The absolute package directory |
There was a problem hiding this comment.
You didn't touch this in this PR, but it causes the new code below to be somewhat needlessly verbose:
It is almost always a bad idea to have & to the owned versions of sequences, e.g. &Vec<T>, &String, &PathBuf.
You don't get any extra powers over just using the borrowed version, i.e. &[T], &str, &Path, because you still can't mutate/move the thing. You still need to drag that lifetime parameter around, and you have a double indirection (in terms of pointers) when a single one would've sufficed.
And as you see in the code below, you now need some extra verbosity to turn &Path into &PathBuf at various points.
I'd suggest restricting yourself to using only PathBuf and &'a Path.
There was a problem hiding this comment.
Good point! I now removed this struct entirely, it wasn't necessary anymore anyways, but this is good to remember for the future.
| second: r.file_name(), | ||
| } | ||
| .into_result::<()>() | ||
| }); |
There was a problem hiding this comment.
Since itertools are already pulled in, could you use duplicates_by here to make it a bit cleaner?
Maybe the Hash bound is a problem though.
There was a problem hiding this comment.
It looks like the duplicates_by iterator only contains one duplicate and not two, which we need for the error here, so this doesn't quite work unfortunately.
Co-authored-by: Wanja Hentze
Was not really necessary anymore
delroth
added
the
12.approvals: 1
3 participants
Description of changes
This is motivated by #256788 (comment), which would be hard to implement in the current code base. Also this just makes things nicer.
This will be essentially a rewrite of the tool, but without any functional changes, I won't touch the test suite.
The commits are kept fairly small and are decently reviewable. Not crucial for anybody to take a super close look though, the tests pass for each commit.
Thought: This should really live in a separate repository..
Ping the other members of the NAT for review: @roberth, @Ericson2314, @tomberek, @DavHau, @aakropotkin, @phaer
This work is sponsored by Antithesis ✨
Things done
nix-build -A tests.nixpkgs-check-by-namesuccessfully onx86_64-linux