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

gtk+: refactor name #100947

Merged
merged 3 commits into from Oct 20, 2020
Merged

gtk+: refactor name #100947

merged 3 commits into from Oct 20, 2020

Conversation

@pickfire
Copy link
Contributor

@pickfire pickfire commented Oct 18, 2020

Use newer pname + version instead of name, I will be using this
when packaging lxde.

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.
Use newer pname + version instead of name, I will be using this
when packaging lxde.
Copy link
Member

@davidak davidak left a comment

LGTM

build would need over 100 GB to download alone, so i don't do that

@jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Oct 18, 2020

What is the point of doing that for unmaintained package that will not receive updates?

@pickfire
Copy link
Contributor Author

@pickfire pickfire commented Oct 18, 2020

I wanted to do gtk = gtk3 rather than withGtk3, I think the first option is cooler.

@7c6f434c
Copy link
Member

@7c6f434c 7c6f434c commented Oct 19, 2020

Stupid question: if you need just pname and version to be available to downstream users, maybe set them in passthru to avoid rebuilds?

@pickfire
Copy link
Contributor Author

@pickfire pickfire commented Oct 19, 2020

But why should it rebuild? Isn't name the same as pname-version? I look at passthru but doesn't this means that we need to duplicate the stuff like pname and version twice?

@7c6f434c
Copy link
Member

@7c6f434c 7c6f434c commented Oct 19, 2020

@jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Oct 19, 2020

If we merge this to staging, rebuild will not be an issue. It will likely be rebuilt anyway.

To clarify, I am not opposed to this, just wanted to know the reason.

@pickfire
Copy link
Contributor Author

@pickfire pickfire commented Oct 19, 2020

@jtojnar I am trying to check gtk.pname == gtk3.pname but it will fail when users pass in gtk2 as gtk since pname is not there, also I can't do gtk == gtk2 since the other stuff may change so matching on pname is the best idea, I also don't want to get the version involved such that I cannot use name.

@7c6f434c I did the method you mentioned and the hash is still the same but @jtojnar say rebuild is not an issue so maybe I can make that change only when it is needed.

@7c6f434c
Copy link
Member

@7c6f434c 7c6f434c commented Oct 19, 2020

@jtojnar @pickfire I believe if the hash is the same, it is a pure information addition with zero rebuild and safe to go directly into master

@7c6f434c
Copy link
Member

@7c6f434c 7c6f434c commented Oct 19, 2020

In principle it is possible to use (builtins.parseDrvName gtk).name, but I agree just adding pname sounds good

@pickfire
Copy link
Contributor Author

@pickfire pickfire commented Oct 20, 2020

@7c6f434c So I need to change it to use passthru?

@7c6f434c
Copy link
Member

@7c6f434c 7c6f434c commented Oct 20, 2020

Use passthru to prevent rebuild.
@pickfire
Copy link
Contributor Author

@pickfire pickfire commented Oct 20, 2020

Done, I added passthru.

Hopefully will satisfy auto-checks…
@7c6f434c 7c6f434c merged commit 86b885a into NixOS:master Oct 20, 2020
18 checks passed
@7c6f434c
Copy link
Member

@7c6f434c 7c6f434c commented Oct 20, 2020

Cleaned up a bit of trailing whitespace that broke the style check. Not worth a round-trip, but I guess worth mentioning as it is now hard to find.

@pickfire pickfire deleted the patch-2 branch Oct 20, 2020
sdier added a commit to sdier/nixpkgs that referenced this issue Oct 23, 2020
@j0xaf
Copy link
Contributor

@j0xaf j0xaf commented Nov 5, 2020

Build is not working for me since this was merged. I get

error: attribute 'passthru' at /nix/store/wi4k17dgd5yj7gg6lb0smc9pflgfm5zi-nixpkgs-21.03pre250093.0da76dab4c2/nixpkgs/pkgs/development/libraries/gtk/2.x.nix:80:3 already defined at /nix/store/wi4k17dgd5yj7gg6lb0smc9pflgfm5zi-nixpkgs-21.03pre250093.0da76dab4c2/nixpkgs/pkgs/development/libraries/gtk/2.x.nix:23:3

@jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Nov 5, 2020

Fixed in 79b173c. Looks like we all, including CI use recent version of Nix that supports merging keys occurring multiple times.

It is a good example why we should follow standard ordering of attributes https://discourse.nixos.org/t/document-attribute-ordering-in-package-expressions/4887.

@jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Nov 5, 2020

I also removed the passthru on staging (64b75ad) since GTK2 is unmaintained upstream and there will not be any future updates.

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

Successfully merging this pull request may close these issues.

None yet

5 participants