NixOS / nixpkgs

Nix Packages collection & NixOS
MIT License
18.36k stars 14.32k forks source link

Tracking issue: superfluous use of `pname` #277994

Open AndersonTorres opened 11 months ago

AndersonTorres commented 11 months ago

There is a huge quantity of appearances of patterns like . . . = pname, . . . = finalAttrs.pname, ${pname} and alike in Nixpkgs.

After some amount of ~heated~ discussions with @SuperSandro2000, I come to the conclusion that this should be discouraged.

There are some reasons I can enumerate for it:

  1. Many, if not all, appearances of pname as an rvalue are basically constants - indeed they are constants since Nix is a functional language. They can be hardcoded with no loss (and some very small performance gains since the compiler will not need to consult the symbol tables).

    • It can be argued that other attributes change constantly, like the ubiquitous version. However, they change based on a familiar modification pattern from the upstream project, whereas things like pname rarely change.
  2. Related to the point above, this use of pname creates otherwise superfluous in-code dependencies

    1. This overcomplicates the static reasoning about the code
    2. This overcomplicates the use of override* functionalities
    3. The issues above are especially more pronounced in the context of finalAttrs design pattern
  3. Sometimes - indeed, most of the time - things like GitHub's repo and other source origins are not identical to pname. Some examples:

    • OpenMSX and Bochs.
      • By convention pname is all-lowercase.
        • An exception consecrated by the use is, curiously, SDL.
    • In the old times, PIL - the Python Imaging Library - had pname = pil and src.url = ". . ./Imaging-${version}.tar.gz"
    • Before migration to a dedicated forge, the MIR Project was called Unity-8. After being passed from Canonical to the community, it was renamed Lomiri, but the forge did not change his name. Indeed the old, now-frozen repo is still called https://github.com/ubports/unity8. Now it lives over https://gitlab.com/ubports/development/core/lomiri
  4. Nix is not a macro language like M4 or the CPP preprocessor.

    1. Just because two things have the same value, we should not treat them as if they are somehow connected
    2. Just because two things are somehow connected, this connection should not necessarily be converted into the code
    3. Pushing to the absurd and linking to the point above, it would be insane to parameterize each occurrence of pname.

References:

oxalica commented 11 months ago

3. 3. This overcomplicates the use of override* functionalities

Why? If we are using rec { .. }, referencing pname explicitly NOT carries any runtime dependency, and override will not affect them. I can understand you want them to be explicitly independent. But I'd argue that only repo = finalAttrs.pname deserves a mass fix, and I cannot find any of these in #278611 after a quick glance.

I also want to list some counter reasons:

  1. Nix has variables, so just use it.
  2. The repository name is expected to be the project name, and pname is also expected to be the project name, at most differs in prefix or postfix (eg. llvm - llvm-project, libfuse - fuse). So they are expected to be connected.
  3. It's common for them to be the same. I failed to randomly come up a package with repository name totally unrelated to its pname.

    A typical example is OpenMSX.

    Fortunately (unfortunately?), GitHub repo name and user name are case-insensitive. I've seen nixos/nixpkgs a lot, even inside nixpkgs. Ideally, they should be corrected, but they does NOT cause any actual issue except for ones with OCD (like me?). Mass trivial changes are not ideal either.

  4. More repetition = more error possibilities. We have to write the package name 4 times with all-packages.nix (2 in all-packages, 1 for file name, 1 in pname), and 2 times with by-name (1 for file name, 1 in pname). Spliting repo and pname will bring it back to 3 times.
eclairevoyant commented 11 months ago

pname and attr name are packaging abstractions, repo name is whatever upstream decided to name them; we should not couple these.

If there's a typing error, it will fail at buildtime anyway.

And I do find 61 examples of the objectively worse repo = finalAttrs.pname.

AndersonTorres commented 11 months ago

and I cannot find any of these in https://github.com/NixOS/nixpkgs/pull/278611 after a quick glance.

Because it is just the round I. I can include them later.

. . . at most differs in prefix or postfix (eg. llvm - llvm-project, libfuse - fuse).

Many thanks to provide even more examples for pname != repo.

So they are expected to be connected.

Not all connections need to be or should be encoded.

Fortunately (unfortunately?), GitHub repo name and user name are case-insensitive.

Neither Nixpkgs is bound by GitHub nor the world of source forges. Just look at the fetchers we have.

Further, there are many other contexts in which we can't assume the URLs are case-insensitive. Indeed we can suppose very few about case sensitiveness of URLs:

From Stack Overflow: Domain names are case insensitive according to RFC 4343. The rest of URL is sent to the server via the GET method. This may be case sensitive or not.

  1. More repetition = more error possibilities. We have to write the package name 4 times with all-packages.nix (2 in all-packages, 1 for file name, 1 in pname), and 2 times with by-name (1 for file name, 1 in pname). Spliting repo and pname will bring it back to 3 times.

The examples you provided are not expected to change in a long term. Hardcoding them brings no harm, even for the Mousepad users. Further, I subscribe the arguments from eclairevoiyant here.

Further, pushing it a bit far, e.g. interpolating every appearance of pname in meta.{changelog,longDescription,homepage, downloadPage} would be a really bad idea.

What about Bochs? Should we interpolate it like

  pname = "bochs";
  repoOwner  = pname + "-emu";
  repoName = 
    let
      splitted = builtins.match "(.)(.*)" pname;
    in
      concatStringsSep "" (singleton (toUpper (head splitted)) ++ (tail splitted));

Remembering that the string "Bochs" appears on the longDescription and is connected to pname -- so just use it?

oxalica commented 11 months ago

And I do find 61 examples of the objectively worse repo = finalAttrs.pname.

They definitely should be fixed.

Further, pushing it a bit far, e.g. interpolating every appearance of pname in meta.{changelog,longDescription,homepage, downloadPage} would be a really bad idea.

What about Bochs? Should we interpolate it like

Of course they shouldn't. As you said, don't make it a macro language, interpolating pname is no good. It's better to write distinct literals for libfuse and llvm-org.

My point is repo = pname is already an explicit way to say "here, (but may not be true for others), we have repo the same as pname", just like repo = "name" says "here, (but may not be true for others), this package is called name".

Further, there are many other contexts in which we can't assume the URLs are case-insensitive.

I agree.

AndersonTorres commented 11 months ago

They definitely should be fixed.

Ideally the finalAttrs should be the standardized, as opposed to rec:

https://github.com/NixOS/nixpkgs/pull/119942

Nowadays the finalAttrs design pattern was implemented only for stdenv.mkDerivation, but ideally it should be the standard - and then all appearances of = pname would become = finalAttrs.pname.

It looks for yet another reason to get rid of both rec and pname.

oxalica commented 11 months ago

Ideally the finalAttrs should be the standardized, as opposed to rec:

119942

It looks for yet another reason to get rid of both rec and pname.

rec and finalAttrs are two orthogonal features. Yes, someone is using them incorrectly and should be fixed. But when you try to replace intended non-overriding rec with overriding finalAttrs (or vice versa), it's introducing a new bug and fix itself in a new way. The issue of mindlessly replacing rec with finalAttrs is exactly what causing this particular issue, and is observed in the PR your mentioned https://github.com/NixOS/nixpkgs/pull/119942/files#r801044378.

Nowadays the finalAttrs design pattern was implemented only for stdenv.mkDerivation, but ideally it should be the standard - and then all appearances of = pname would become = finalAttrs.pname.

Don't introduce another with, please.

peterhoeg commented 10 months ago

This has been a pet peeve of mine for a long time, so I for one would like to say thank you for moving this ahead!

eclairevoyant commented 4 months ago

I do find 61 examples of the objectively worse repo = finalAttrs.pname.

Decided to finally fix them, in #326683.

dotlambda commented 3 months ago

Is there agreement on how to treat fetchPypi? Since the pname of a Python package is supposed to be identical, barring PEP 503 normalization, to its name on PyPI, I'd argue that use of inherit pname is warranted in fetchPypi.

eclairevoyant commented 3 months ago

I've personally treated it as an exception; eventually we may even want to make it easier to normalise already so that package names with underscores, etc. can avoid having to write those out twice.

dotlambda commented 3 months ago

eventually we may even want to make it easier to normalise already

You mean buildPythonPackage should normalize pname automatically?

eclairevoyant commented 3 months ago

My suggestion was offtopic for here, but yes

fricklerhandwerk commented 4 days ago

Can this please be added to the contributor guide? And if there's still contention, run it through the RFC process first?