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

Error messages should be better #6

Open
infinisil opened this issue Feb 5, 2024 · 4 comments
Open

Error messages should be better #6

infinisil opened this issue Feb 5, 2024 · 4 comments

Comments

@infinisil
Copy link
Member

infinisil commented Feb 5, 2024

It should be better explained what people can do when certain CI errors occur. I'm currently manually writing comments like this:

And I shouldn't have to if CI errors were better and ideally linked to more thorough documentation :)

@justinas
Copy link
Member

justinas commented Feb 5, 2024

TBH the errors I encountered in this were not hard to comprehend or anything. It was just not straightforward to me how to handle the transition to by-name for packages with multiple versions (pkg_1, pkg_2, etc.), and this was left undecided in RFD 140 itself.

Your comment is super helpful though 🙌

@infinisil
Copy link
Member Author

Just opened NixOS/nixpkgs#292214 to document recommendations for adding new package versions

And there's NixOS/nixpkgs#290743 to improve other error messages

@infinisil infinisil transferred this issue from NixOS/nixpkgs Mar 18, 2024
@infinisil infinisil changed the title pkgs/by-name error messages should be better Error messages should be better Mar 26, 2024
@willbush
Copy link
Member

willbush commented Apr 15, 2024

I was a little curious to do some analysis on failed runs of the check-by-name.yml workflow in the nixpkgs repository.

Here's a way to download the logs of the failed runs using Github cli:

run from the nixpkgs/.github/workflows master branch:

gh run list --workflow=check-by-name.yml --status=failure --json databaseId --limit 1000 |
  jq -r '.[] | .databaseId' |
  xargs -P 4 -I{} sh -c '
    echo "Processing run ID {}..."
    gh run view {} --log-failed > "run_{}.log"
  '

Note it's important to be aware of the rate limit of the Github API, you can check it by running:

❯ gh api rate_limit | jq '.rate'
{
  "limit": 5000,
  "used": 1,
  "remaining": 4999,
  "reset": 1713159188
}

My initial testing was with -P 16 and I hit the rate limit quickly.

I changed it to -P 4 and it uses up about 3k requests:

❯ gh api rate_limit | jq '.rate'
{
  "limit": 5000,
  "used": 3013,
  "remaining": 1987,
  "reset": 1713159188
}

Results

I moved the logs to a separate directory and removed the empty logs:

❯ mkdir -p ~/check-by-name-failed
❯ mv run_* ~/check-by-name-failed
❯ cd ~/check-by-name-failed
❯ ls *.log -l | wc -l
1000
❯ find . -name "*.log" -type f -empty -delete
❯ ls *.log -l | wc -l
984

nix eval errors

❯ mkdir -p ~/processed
❯ rg -l "nix-instantiate" | wc -l
319
❯ rg -l "nix-instantiate" | xargs -I {} mv {} ~/processed
❯ rg -l "Nix evaluation failed for some package" | wc -l
38
❯ rg -l "Nix evaluation failed for some package" | xargs -I {} mv {} ~/processed

Seems to be 357 logs that relate to nix eval errors which makes sense to me.
Just wondering what GH action caught this before check-by-name?

Actually, I might have screwed up somewhere? After processing more logs I found
more nix eval errors. So perhaps it's even more.

new top-level packages

❯ rg -l "top-level package" | wc -l
314
❯ rg -l "top-level package" | xargs -I {} mv {} ~/processed

Github error limit

❯ rg -l "Not retrying anymore, probably GitHub is having internal issues" | wc -l
132
❯ rg -l "Not retrying anymore, probably GitHub is having internal issues" | xargs -I {} mv {} ~/processed

missing package.nix file

❯ rg -l "Missing required \"package.nix\" file" | wc -l
41
❯ rg -l "Missing required \"package.nix\" file" | xargs -I {} mv {} ~/processed

must be defined like

❯ rg -l "must be defined like" | wc -l
44
❯ rg -l "must be defined like" | xargs -I {} mv {} ~/processed

example of multiple similar errors in a single file:

- Because pkgs/by-name/mi/microchip-xc-dsc exists, the attribute `pkgs.microchip-xc-dsc` must be defined like

    microchip-xc-dsc = callPackage ./pkgs/by-name/mi/microchip-xc-dsc/package.nix { /* ... */ };

  However, in this PR, the second argument is empty. See the definition in pkgs/top-level/all-packages.nix:33308:

    microchip-xc-dsc = callPackage ../by-name/mi/microchip-xc-dsc/package.nix { };

  Such a definition is provided automatically and therefore not necessary. Please remove it.

- Because pkgs/by-name/mi/microchip-xc16 exists, the attribute `pkgs.microchip-xc16` must be defined like

    microchip-xc16 = callPackage ./pkgs/by-name/mi/microchip-xc16/package.nix { /* ... */ };

  However, in this PR, the second argument is empty. See the definition in pkgs/top-level/all-packages.nix:33304:

    microchip-xc16 = callPackage ../by-name/mi/microchip-xc16/package.nix { };

  Such a definition is provided automatically and therefore not necessary. Please remove it.

- Because pkgs/by-name/mi/microchip-xc32 exists, the attribute `pkgs.microchip-xc32` must be defined like

    microchip-xc32 = callPackage ./pkgs/by-name/mi/microchip-xc32/package.nix { /* ... */ };

  However, in this PR, the second argument is empty. See the definition in pkgs/top-level/all-packages.nix:33306:

    microchip-xc32 = callPackage ../by-name/mi/microchip-xc32/package.nix { };

  Such a definition is provided automatically and therefore not necessary. Please remove it.

- Because pkgs/by-name/mi/microchip-xc8 exists, the attribute `pkgs.microchip-xc8` must be defined like

    microchip-xc8 = callPackage ./pkgs/by-name/mi/microchip-xc8/package.nix { /* ... */ };

  However, in this PR, the second argument is empty. See the definition in pkgs/top-level/all-packages.nix:33302:

    microchip-xc8 = callPackage ../by-name/mi/microchip-xc8/package.nix { };

  Such a definition is provided automatically and therefore not necessary. Please remove it.

This PR introduces additional instances of discouraged patterns as listed above. Merging is discouraged but would not break the base branch.
To run locally: ./maintainers/scripts/check-by-name.sh master https://github.com/NixOS/nixpkgs.git

rnix errors

❯ rg -l rnix | wc -l
24
❯ rg -l rnix | xargs -I {} mv {} ~/processed

Note: I did not finish processing and categorizing all log files. The above are the largest categories.

@philiptaron
Copy link
Contributor

This is a great data driven approach to improving the errors. Thanks for documenting your process, @willbush!

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

No branches or pull requests

4 participants