search

Mic92 / nixpkgs-review

Review pull-requests on https://github.com/NixOS/nixpkgs
MIT License
380 stars 63 forks source link

Feature request: ability to post/report merge failures and other evaluation errors #275

Closed pbsds closed 2 years ago

pbsds commented 2 years ago

When looking at older PRs i sometimes run into git merge conflicts. The current behavior for nixpkgs-review seems is to skip the PRs that fail to merge, never opening them in an interactive shell. Is it possible to add a feature where nixpkgs-review posts to github that the PR fails merging into master?

Currently I read the nixpkgs-review output and post comments such as these manually: https://github.com/NixOS/nixpkgs/pull/153870#issuecomment-1207291016 https://github.com/NixOS/nixpkgs/pull/175034#issuecomment-1207293004

SuperSandro2000 commented 2 years ago

I think that is a bit out of scope for nixpkgs-review. GitHub already displays if there is a merge conflict in the web ui. The author does not receive a notification for that, so sometimes they need to be reminded about that.

pbsds commented 2 years ago

Perhaps a merge conflict is out of scope, as the github ui already checks for mergeability. But nix evaluation errors, like the second comment linked, i believe is in scope. Perhaps the request for merge conflicts to be posted is too specific, but what if i generalize my feature request to cover "any reason nixpkgs-review could not enter the nix-shell environment"?

SuperSandro2000 commented 2 years ago

For that the ofborg checks already exists which are stricter than nixpkgs-review. I don't think we should replicate them.

I don't think it is very useful to post failures directly on GitHub. nixpkgs-review can fail for many reasons like being OOM killed and entering the shell can fail for known reasons like multiple QT versions being used. If we don't fix those errors first or at least can reliable filter them then this feature would not be to useful for the PR author.