Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

github PRs: utilize nixpkgs-lint #11265

Closed
vcunat opened this issue Nov 25, 2015 · 10 comments
Closed

github PRs: utilize nixpkgs-lint #11265

vcunat opened this issue Nov 25, 2015 · 10 comments

Comments

@vcunat
Copy link
Member

vcunat commented Nov 25, 2015

It would be nice if it auto-reminded of missing or bad meta-data. (Follow up from #9966.)

@pSub
Copy link
Member

pSub commented Nov 25, 2015

This is also covered in #11166. I will try to tackle this in the next days.

@domenkozar
Copy link
Member

🍰 I know that @mbbx6spp might be working on a linter :)

@pSub
Copy link
Member

pSub commented Nov 27, 2015

For the integration into Github I see the following possibilities:

For all existing solution we would have to implement the integration with nixpkgs-lint.

@mbbx6spp what are you working on concretely?

@pSub
Copy link
Member

pSub commented Dec 5, 2015

Update: I am currently working on nixpkgs-lint integration into markstory/lint-review. I hope I have some results tomorrow.

@grahamc
Copy link
Member

grahamc commented Feb 22, 2016

I have lint running on every PR on my personal build server. I've been working to leave feedback on PRs when I notice it reporting a degradation, but it is a bit tricky to detect which package(s) were changed and if the change was for a good / worse. Right now I have it with a diff (a bit hacky...):

#!/bin/sh

set -eux

git fetch origin

# $upstream is `master` or `release-15.09`, the branch being merged in to
git checkout "origin/$upstream"


nixpkgs-lint -f . > lint-from-target-branch

git merge "$current_branch"
nixpkgs-lint -f . > lint-from-pr

diff lint-from-target-branch lint-from-pr

If there is a good way to determine which packages need to be run against, I'd be happy to improve it and more readily leave feedback.

@pSub
Copy link
Member

pSub commented Feb 22, 2016

I do not have a solution for finding the packages that have changed. However, I do no think that it is necessary to have this information. My current plan is to modify nixpkgs-lint s.t. it prints information about the position of the problem (path to the file and line number) and then append this information to the PR via a tool like lint-review.

This looks like

pkgs/misc/beep/default.nix:20: Lacks a maintainer
pkgs/misc/beep/default.nix:20: Description contains package name

The modification of nixpkgs-lint is nearly done and I will push it in the next ~2 days. I also hope to have enough time to set up lint-review to work with nixpkgs-lint next week.

@matthewbauer
Copy link
Member

matthewbauer commented Jul 7, 2016

@pSub Is this still a work in progress? We can get Travis to run "nixpkgs-lint" but it won't be well formatted.

If we can get "lint-review" in nixpkgs, could we have it run on one of the Hydra machines?

matthewbauer added a commit to matthewbauer/nixpkgs that referenced this issue Jul 11, 2016
This will run nixpkgs-lint for each travis build.

Maybe??? fixes NixOS#11265
@cmfwyp
Copy link
Member

cmfwyp commented Aug 3, 2016

Followup to my suggestion on #17458 to only check modified packages. The nixpkgs-lint script needs a package name, so we just need a way to determine which packages have been modified by a commit. This could be done by getting the paths of Nix expressions for modified packages from git diff --numstat HEAD~1 | grep -o 'pkgs/.*\.nix', extracting the package name from the files by capturing it with name = "(.+)";, then passing the name to nixpkgs-lint with the --package option.

Only two issues: commits in branches other than master, which would require finding that branch and doing a git checkout first, and files that contain lines matching name = "(.+)"; where the capture isn't actually a package name, if these exist.

Adding this to travis-nox-review-pr.sh might work:

for line in "$(git diff --numstat HEAD~1 | grep -o 'pkgs/.*\.nix')"
do
    nix-shell --packages nixpkgs-lint --run "nixpkgs-lint -f $TRAVIS_BUILD_DIR -p $(grep -o 'name = ".*";' $line | tail -c +9 | head -c -2)"
done

@Profpatsch
Copy link
Member

Also see #37252 which was created to be able to type-check arbitrary (non-contravariant, aka no functions) nix data.

There is a working proof of concept for checking the whole of meta and the contents of derivations, you can find it here and see the types and the result of one such evaluation here.

@pSub pSub closed this as completed Jul 15, 2018
@Profpatsch
Copy link
Member

@pSub Are you closing because it is fixed or because you stopped working on it? Since we have ofborg now, it seems to be more relevant now then when the issue was opened.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants