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

Treewide: change all occurrences of "name" to "pname-version" #104045

Closed
wants to merge 0 commits into from
Closed

Treewide: change all occurrences of "name" to "pname-version" #104045

wants to merge 0 commits into from

Conversation

AndersonTorres
Copy link
Member

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@AndersonTorres
Copy link
Member Author

Related issue: #103997

@siraben
Copy link
Member

siraben commented Nov 17, 2020

Is it expected that none of these derivations will build differently because the interpolated strings are the same as they were before?

@AndersonTorres
Copy link
Member Author

Yes. It is just a rewriting of the expressions.

@@ -14,7 +14,7 @@ stdenv.mkDerivation rec {
# which will contain the links to all available *.debs for the arch.

src = fetchurl {
url = "http://dl.google.com/linux/musicmanager/deb/pool/main/g/google-musicmanager-beta/${name}_amd64.deb";
url = "http://dl.google.com/linux/musicmanager/deb/pool/main/g/google-musicmanager-beta/${pname}-${version}_amd64.deb";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated to this PR but the platform should probably be set to x84_64 only.

name = "jackmix-0.5.2";
pname = "jackmix";
version = "0.5.2";

src = fetchurl {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably use fetchFromGitHub.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would change the final output. I am not trying to do this here and now.

src = fetchurl {
url = "mirror://sourceforge/mp3gain/mp3gain-1_6_2-src.zip";
url = "mirror://sourceforge/mp3gain/${pname}-1_6_2-src.zip";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
url = "mirror://sourceforge/mp3gain/${pname}-1_6_2-src.zip";
url = "mirror://sourceforge/mp3gain/${pname}-${builtins.replaceStrings ["."] ["_"] version}-src.zip";

I did not test this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also out of scope for this PR.

Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't eval for the most part.

Fix: https://github.com/AndersonTorres/nixpkgs/pull/1

@Atemu
Copy link
Member

Atemu commented Nov 17, 2020

With my changes, Nixpkgs can be eval'd again and the sources that aren't already broken still produce the same output.

Also, nit: Treewide -> treewide

@AndersonTorres
Copy link
Member Author

Oh man, this will not be easy.

There are many "legitimate" uses of namein Nixpkgs, in the sense that merely changing a single file from name to pname-version triggers auxiliary functions defined elsewhere - and these functions use name!

@doronbehar
Copy link
Contributor

I think it's impossible for this to not trigger rebuilds since even in the simplest case, the builder gets after this change the new environment variable pname and version which weren't there before and unfortunately (without structuredAttrs I suppose) it's a change in the inputs.

@AndersonTorres
Copy link
Member Author

@doronbehar : then, in a certain sense, the expression itself becomes a "build input" of itself?

And what is structuredAttrs?

Well, in this sense I think we can sprint it to the next viable update.

@doronbehar
Copy link
Contributor

And what is structuredAttrs?

#72074 && https://nixos.mayflower.consulting/blog/2020/01/20/structured-attrs/

@doronbehar : then, in a certain sense, the expression itself becomes a "build input" of itself?

I'm not sure I understand your explanation, but to demonstrate how easily rebuilds can be triggered, consider a change such as:

diff --git i/pkgs/development/compilers/gcc/9/default.nix w/pkgs/development/compilers/gcc/9/default.nix
index 305ed56df78..e39c787284b 100644
--- i/pkgs/development/compilers/gcc/9/default.nix
+++ w/pkgs/development/compilers/gcc/9/default.nix
@@ -83,6 +83,7 @@ in
 stdenv.mkDerivation ({
   pname = "${crossNameAddon}${name}${if stripped then "" else "-debug"}";
   inherit version;
+  dummyVar = "bla";
 
   builder = ../builder.sh;
 

That would rebuild the world, and you can see that via running nix show-derivation -f. gcc.cc before and after the change, and inspect the diff in a text editor.

The difference between the derivations can be found in the env attribute of the derivations, where the dummyVar environment variable which you and I know that won't affect the build is the culprint, but Nix doesn't know it.

In general nix show-derivation -f. is the judge, though ofborg has a more sophisticated way of doing that in a batch.

What's so frustrating about it is that our default-builder.sh and most of it's derivatives handle the conditionals which ideally should have been evaluated by Nix. A real world examples of such unnecessary rebuilds triggered is (only the Darwin rebuilds of) e.g: #94952 (comment) .

Even changes in the order of inputs in a Nix arrays causes rebuilds, consider this change:

diff --git i/pkgs/development/compilers/gcc/9/default.nix w/pkgs/development/compilers/gcc/9/default.nix
index 305ed56df78..d73089e9c0c 100644
--- i/pkgs/development/compilers/gcc/9/default.nix
+++ w/pkgs/development/compilers/gcc/9/default.nix
@@ -168,7 +168,7 @@ stdenv.mkDerivation ({
     ++ optional targetPlatform.isLinux patchelf;
 
   buildInputs = [
-    gmp mpfr libmpc libelf
+    mpfr gmp libmpc libelf
     targetPackages.stdenv.cc.bintools # For linking code at run-time
   ] ++ (optional (isl != null) isl)
     ++ (optional (zlib != null) zlib)

It will also trigger a rebuild of the world.

The problem is that mkDerivation doesn't know how to filter out the unnecessary information for our default-builder.sh though I can't think of a better way to design it.


structuredAttrs could perhaps allow us to write a default-builder.sh that will hopefully be resilient to this false interpretations of changed inputs.

@jtojnar
Copy link
Member

jtojnar commented Nov 19, 2020

@doronbehar order of build inputs matters since it can change the order setup hooks are run and we cannot guarantee arbitrary bash code commutes. Another example where it is a problem are environment variables like PKG_CONFIG_PATH – when multiple different derivations contain the same library, the first one found in the PKG_CONFIG_PATH will be chosen. I routinely encounter this when overriding subset of dependencies with enableDebugging, especially when dependency propagation comes into play.

Copy link
Member

@FRidh FRidh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be merged until consensus is found in #103997.

@AndersonTorres
Copy link
Member Author

AndersonTorres commented Nov 21, 2020

This is just a parallel work for now, and way far from a clear finish.

Anyway I will treat this as Draft for now.

@AndersonTorres AndersonTorres marked this pull request as draft November 21, 2020 15:09
@AndersonTorres AndersonTorres marked this pull request as ready for review November 21, 2020 16:18
@AndersonTorres
Copy link
Member Author

AndersonTorres commented Nov 21, 2020

Another day, another huma nerror. Now I will try to recover.

EDIT: is the interface broken? I can't reopen this!

@Atemu
Copy link
Member

Atemu commented Nov 22, 2020

GH thinks you force-pushed to a commit already in master and, since that means there are no changes, won't let you open the PR.

Should work again if you push another commit.

Would probably also not be a bad idea to simply push commits and then squash them up once they're ready. Following changes between force-pushes isn't really something you can do very well in git.

@SuperSandro2000
Copy link
Member

Would probably also not be a bad idea to simply push commits and then squash them up once they're ready. Following changes between force-pushes isn't really something you can do very well in git.

Not true as long as you don't do a rebase in the same force push. https://github.com/NixOS/nixpkgs/compare/32986af10085058e08125720467d84f5e1afe4b3..712c2405b1c4ddf461a1eafd640165583d1c0cca

@AndersonTorres
Copy link
Member Author

Reopening
#104662

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

Successfully merging this pull request may close these issues.

7 participants