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

Jump to bottom

cinnamon.xapps: init at 1.6.8 #75138

Merged
merged 3 commits into from Dec 15, 2019
Merged

cinnamon.xapps: init at 1.6.8 #75138

merged 3 commits into from Dec 15, 2019

Conversation

mkg20001
Copy link
Member

@mkg20001 mkg20001 commented Dec 7, 2019

Motivation for this change

This adds the xapps package and it's python module to nixpkgs, to allow for the cinnamon desktop to be ported

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 nix-review --run "nix-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.
Notify maintainers

cc @

@mkg20001 mkg20001 mentioned this pull request Dec 7, 2019
Closed
10 tasks
worldofpeace
worldofpeace suggested changes Dec 7, 2019
View reviewed changes
Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

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

Could you style your expressions like in?

Most notably,

{ fetchFromGitHub
, something
, etc
}:

and arrays with one item per line.

For the python library, you have hardcoded binaries you have to patch

In this particular case it's trying to access some optional binaries.
I recommend only patching the code for polkit, as you could easily control that configuration within the cinnamon module. (and it would be preferred).

It also seems to not be patched properly by python setup hooks, and even has an FHS shebang in one file. Will look into that later.

pkgs/desktops/cinnamon/default.nix Outdated Show resolved Hide resolved
pkgs/desktops/cinnamon/xapps/default.nix Outdated Show resolved Hide resolved
pkgs/desktops/cinnamon/xapps/default.nix Outdated Show resolved Hide resolved
pkgs/desktops/cinnamon/xapps/default.nix Outdated Show resolved Hide resolved
pkgs/desktops/cinnamon/xapps/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/xapp/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/xapp/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/xapp/default.nix Outdated Show resolved Hide resolved
pkgs/desktops/cinnamon/xapps/default.nix Outdated Show resolved Hide resolved
@mkg20001 mkg20001 force-pushed the feat/add-xapps-cinnamon branch 4 times, most recently from 1bd50a1 to 033a87b Compare December 7, 2019 16:45
jonringer
jonringer reviewed Dec 8, 2019
View reviewed changes
pkgs/desktops/cinnamon/xapps/default.nix Outdated Show resolved Hide resolved
pkgs/desktops/cinnamon/xapps/default.nix Outdated Show resolved Hide resolved
jonringer
jonringer reviewed Dec 8, 2019
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

please squash the commits, I would squash your cinnamon namespace commit with xapps, then have python as a separate commit

cinammon.xapps: init at 1.6.4
pythonPackages.xapps: init at 1.6.3

@mkg20001 mkg20001 force-pushed the feat/add-xapps-cinnamon branch 7 times, most recently from f216528 to aa3c493 Compare December 10, 2019 09:41
@mkg20001
Copy link
Member Author

IMO this is done so far. Only thing left is some unanswered questions in the review-comments & the upstream patch for glib not being found.

@mkg20001 mkg20001 force-pushed the feat/add-xapps-cinnamon branch 2 times, most recently from 4945aa1 to 428e988 Compare December 13, 2019 19:36
jonringer
jonringer approved these changes Dec 13, 2019
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

LGTM

[9 built, 12 copied (3.8 MiB), 0.8 MiB DL]
https://github.com/NixOS/nixpkgs/pull/75138
4 package were built:
cinnamon.xapps python27Packages.xapp python37Packages.xapp python38Packages.xapp

@jonringer
Copy link
Contributor

@GrahamcOfBorg build cinnamon.xapps python27Packages.xapp python37Packages.xapp python38Packages.xapp

@jonringer
Copy link
Contributor

@worldofpeace you still have a "change request", are you good with this?

@worldofpeace
Copy link
Contributor

I believe #75138 (comment) broke things.
The package would probably propagate python, but not a python with the package in site-packages. So everything gets patchShebang'd with a plain python. That's what withPackages is for, but we still need the package in buildInputs because it needs the .pc for meson.

@jonringer
Copy link
Contributor

oh, well, it's currently in buildinputs, which wouldn't get propagated anyway. If it's just needed for meson, then shouldn't that go in nativeBuildInputs?

@worldofpeace
Copy link
Contributor

worldofpeace commented Dec 13, 2019 •
edited

oh, well, it's currently in buildinputs, which wouldn't get propagated anyway. If it's just needed for meson, then shouldn't that go in nativeBuildInputs?

It's needed for meson, we missed this because it was a runtime dependency.
I checked and I don't see it in a shebang so we're good https://github.com/linuxmint/xapps/search?utf8=%E2%9C%93&q=python&type=. But python should be in nativeBuildInputs

@mkg20001
Copy link
Member Author

Which python should be in native build inputs? The one with withPackages or the python3Packages.pygoobject3 @worldofpeace

@mkg20001
Copy link
Member Author

I've noticed that upload-system-info was referencing pastebin via a non-nix fixed path, fixed that

It also uses inxi from $PATH. How should that be handled? Include it as a buildInput? Or just add it to the system via the cinnamon nixos module?

@jonringer
Copy link
Contributor

if you want it to remain in python-modules, so that it can be used by other packages, you need to patch the call. Something like popen("inixi",...) -> popen("${inixi-package}/bin/inxi",...

Patching is required because you can't count on any type of wrapping :(

If it's an application (and not meant to be imported by other packages), then you can do as much wrapping as you would like

@mkg20001
Copy link
Member Author

upload-system-info is part of xapps, not python-xapp

@jonringer
Copy link
Contributor

oh, for xapps? then you can use makeWrapper, here's an example for minecraft:

nixpkgs/pkgs/games/minecraft/default.nix

Line 113 in 9ace40c

makeWrapper $out/opt/minecraft-launcher/minecraft-launcher $out/bin/minecraft-launcher \

@mkg20001
Copy link
Member Author

@jonringer I've ended up directly patching the location, since the executables are already wrapped and double-wrapping seems a tad too much

@worldofpeace Is pygobject3 just a build dependency? The scripts seem to require python, so unsure what to do here.

@worldofpeace worldofpeace changed the title cinnamon.xapps: init at 1.6.4 cinnamon.xapps: init at 1.6.8 Dec 15, 2019
@worldofpeace worldofpeace merged commit bfcc281 into NixOS:master Dec 15, 2019
@worldofpeace
Copy link
Contributor

@mkg20001 Our reviews become divisive here. Anything that was an issue I fixed in the force push.
If you'd like an explanation please PM on @worldofpeace on Freenode or Matrix/Riot, or in #nixos-dev.

@mkg20001 mkg20001 deleted the feat/add-xapps-cinnamon branch June 28, 2021 04:09
@mkg20001 mkg20001 restored the feat/add-xapps-cinnamon branch June 28, 2021 04:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonringer jonringer jonringer approved these changes

@FRidh FRidh Awaiting requested review from FRidh

@worldofpeace worldofpeace Awaiting requested review from worldofpeace

@hedning hedning Awaiting requested review from hedning

@jtojnar jtojnar Awaiting requested review from jtojnar

Assignees
No one assigned
Labels
6.topic: python 8.has: package (new) 10.rebuild-darwin: 0 10.rebuild-linux: 11-100 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@mkg20001 @worldofpeace @jonringer @jtojnar @nixos-discourse