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

Choose a reason for hiding this comment

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

LGTM

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

@jtojnar
Copy link
Contributor

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 commented Oct 18, 2020

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

@7c6f434c
Copy link
Member

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

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 commented Oct 19, 2020 via email

@jtojnar
Copy link
Contributor

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 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

@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

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

@pickfire
Copy link
Contributor Author

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

@7c6f434c
Copy link
Member

7c6f434c commented Oct 20, 2020 via email

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

Done, I added passthru.

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

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 October 20, 2020 17:04
@j0xaf
Copy link
Contributor

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 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 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
Development

Successfully merging this pull request may close these issues.

None yet

5 participants