More complex `patches` AST structures not necessarily handled well

rmcgibbo commented 3 years ago

It looks like there are some more complicated patches structures (in terms of the AST) out there in the wild that will get incorrectly flagged as "warning: missing-patch-comment patches should be a list."

Example: https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/python-modules/numpy/default.nix#L55-L66:

buildPythonPackage rec {
  ...
  patches = [
    # For compatibility with newer pytest
    (fetchpatch {
      url = "https://github.com/numpy/numpy/commit/ba315034759fbf91c61bb55390edc86e7b2627f3.patch";
      sha256 = "F2P5q61CyhqsZfwkLmxb7A9YdE+43FXLbQkSjop2rVY=";
    })
  ] ++ lib.optionals python.hasDistutilsCxxPatch [
    # We patch cpython/distutils to fix https://bugs.python.org/issue1222585
    # Patching of numpy.distutils is needed to prevent it from undoing the
    # patch to distutils.
    ./numpy-distutils-C++.patch
  ];
}

Perhaps, if patches is not a list, we should just not throw a warning at all...

rmcgibbo commented 3 years ago

Looks like I need to fix this bug since @AndersonTorres encountered it "in the wild".

jtojnar commented 3 years ago

Hmm, would be nice if we could evaluate the patches attribute and then obtain the corresponding AST nodes for the list items. Need to check if it is possible to write a Nix plug-in for that.

rmcgibbo commented 3 years ago

I suppose that's one way to do it. I think the near-term solution is simply to skip the ast-check itself if the node is not of type list.

jtojnar commented 3 years ago

Sounds good.

jtojnar / nixpkgs-hammering

More complex `patches` AST structures not necessarily handled well #23