search

jtojnar / nixpkgs-hammering

Beat your package expressions into a shape
MIT License
245 stars 14 forks source link

attribute-typo: Add more known attributes #38

Closed jtojnar closed 3 years ago

jtojnar commented 3 years ago

https://github.com/jtojnar/nixpkgs-hammering/issues/33

jtojnar commented 3 years ago

Seems to fix the most common cases now.

rmcgibbo commented 3 years ago
SuperSandro2000 commented 3 years ago

Lots of packages use rec attrsets, so these attributes are "used" elsewhere in the attrset passed to makeDerivation. Should the warning message recommend using a let binding or something?

Wouldn't that trigger for all version's which are reused? Usually we do the opposite and move things from let in to rec because it is cleaner.

rmcgibbo commented 3 years ago

@SuperSandro2000: Yeah, totally fair. I'm just wondering if this attribute-typo check is going to be printing a lot of false positives.

rmcgibbo commented 3 years ago

Here's an example of what I mean -- the libPath attribute is flagged as a typo and I'm genuinely not sure if we want it to be: https://github.com/NixOS/nixpkgs/pull/112852#issuecomment-777948585

jtojnar commented 3 years ago
  • Am I correct that these unused attributes end up as environment variables in the build, and if so, they might actually be used? Should we have a special way of treading all caps unknown attributes if so?

Correct. Yeah, I thought about handling all caps differently and concluded that there probably will not be many similar attributes for the env vars (except the shortest ones but we handle those separately now). Eventually, I would want to move env vars into a separate attribute.

  • Lots of packages use rec attrsets, so these attributes are "used" elsewhere in the attrset passed to makeDerivation. Should the warning message recommend using a let binding or something?

Yeah, I think we should move stuff that does not concern stdenv or some setup hook to let to avoid contaminating the environment. Second use case are output attributes and those should go to passthru instead of being passed to stdenv directly.

jtojnar commented 3 years ago

Here's an example of what I mean -- the libPath attribute is flagged as a typo and I'm genuinely not sure if we want it to be: NixOS/nixpkgs#112852 (comment)

Yeah, that libPath is not used by stdenv so I would suggest moving it in let binding in postFixup or moving the libs to buildInputs and adding autoPatchElfHook. (Perhaps that can be a check.)

rmcgibbo commented 3 years ago

Maybe if the arguments passed to stdenv are a rec block and the "possible typo"-d attribute is used elsewhere within the rec block, we ought to be giving a different warning message or recommendation rather than calling it a typo?

jtojnar commented 3 years ago

Yeah, that is pretty reasonable. Unfortunately, it requires a static check so it cannot be done in overlay. (Thought maybe a Nix plug-in could do that.)

For now, maybe the bot could exclude this check? Maybe we could add maturity levels to checks.

rmcgibbo commented 3 years ago

Yeah -- this will be more work to implement and is totally independent of this PR, which I think is good to merge now.