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

Tracking issue: superfluous use of pname #277994

Open
AndersonTorres opened this issue Jan 1, 2024 · 7 comments
Open

Tracking issue: superfluous use of pname #277994

AndersonTorres opened this issue Jan 1, 2024 · 7 comments
Labels
5. scope: tracking Long-lived issue tracking long-term fixes or multiple sub-problems 6.topic: best practices

Comments

@AndersonTorres
Copy link
Member

AndersonTorres commented Jan 1, 2024

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
Copy link
Contributor

oxalica commented Jan 4, 2024

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
Copy link
Member

eclairevoyant commented Jan 4, 2024

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 added a commit to atorres1985-contrib/nixpkgs that referenced this issue Jan 4, 2024
@AndersonTorres
Copy link
Member Author

AndersonTorres commented Jan 4, 2024

and I cannot find any of these in #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
Copy link
Contributor

oxalica commented Jan 4, 2024

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
Copy link
Member Author

They definitely should be fixed.

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

#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
Copy link
Contributor

oxalica commented Jan 4, 2024

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.

AndersonTorres added a commit to atorres1985-contrib/nixpkgs that referenced this issue Jan 4, 2024
AndersonTorres added a commit to atorres1985-contrib/nixpkgs that referenced this issue Jan 6, 2024
AndersonTorres added a commit to atorres1985-contrib/nixpkgs that referenced this issue Jan 6, 2024
AndersonTorres added a commit to atorres1985-contrib/nixpkgs that referenced this issue Jan 6, 2024
AndersonTorres added a commit to atorres1985-contrib/nixpkgs that referenced this issue Jan 6, 2024
AndersonTorres added a commit to atorres1985-contrib/nixpkgs that referenced this issue Jan 6, 2024
AndersonTorres added a commit to atorres1985-contrib/nixpkgs that referenced this issue Jan 6, 2024
AndersonTorres added a commit to atorres1985-contrib/nixpkgs that referenced this issue Jan 6, 2024
AndersonTorres added a commit to atorres1985-contrib/nixpkgs that referenced this issue Jan 6, 2024
AndersonTorres added a commit to atorres1985-contrib/nixpkgs that referenced this issue Jan 7, 2024
AndersonTorres added a commit to atorres1985-contrib/nixpkgs that referenced this issue Jan 7, 2024
AndersonTorres added a commit to atorres1985-contrib/nixpkgs that referenced this issue Jan 7, 2024
@wegank wegank mentioned this issue Jan 7, 2024
13 tasks
This was referenced May 4, 2024
@kirillrdy kirillrdy mentioned this issue May 17, 2024
13 tasks
@RaghavSood RaghavSood mentioned this issue May 20, 2024
13 tasks
@heisfer heisfer mentioned this issue May 30, 2024
12 tasks
@afh afh mentioned this issue Jun 6, 2024
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5. scope: tracking Long-lived issue tracking long-term fixes or multiple sub-problems 6.topic: best practices
Projects
None yet
Development

No branches or pull requests

5 participants