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

wayfire: init at 0.4.0 #86606

Open
wants to merge 5 commits into
base: master
from
Open

wayfire: init at 0.4.0 #86606

wants to merge 5 commits into from

Conversation

@alyssais
Copy link
Member

alyssais commented May 2, 2020

Motivation for this change

I’ve been working this for the past few days independently of #86569, which I just saw. We should compare the two and see if we can combine approaches.

The major difference is that this PR supports Wayfire plugins through a standard Nixpkgs-style wayfire.withPackages interface. Upstream Wayfire only works if plugins are installed into Wayfire’s prefix, which isn’t really an option for us, at least with arbitrary plugins. But the good news is they understand the problem, and are willing to accept my patches. So everything that’s a patch in this PR should hopefully be part of the next Wayfire release, at which point these patches can all be dropped.

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

alyssais commented May 2, 2020

I’ve realised that wcm will need to be wrapped too, so I’d like to come up with some nice function that maybe gives you an attrset with wlc and wayfire packages, both wrapped with the right environment variables set. The hard part is coming up with a name…

@alyssais alyssais force-pushed the alyssais:wayfire branch from 04710f9 to 0ba44c2 May 3, 2020
@alyssais
Copy link
Member Author

alyssais commented May 3, 2020

@Thra11 Thra11 mentioned this pull request May 4, 2020
2 of 10 tasks complete
@wucke13
Copy link
Contributor

wucke13 commented May 4, 2020

You may add me to the maintainers, if you'd like to.

@alyssais alyssais force-pushed the alyssais:wayfire branch from 0ba44c2 to 432eb67 May 7, 2020
@ofborg ofborg bot requested a review from wucke13 May 7, 2020
@alyssais
Copy link
Member Author

alyssais commented May 7, 2020

@wucke13
wucke13 approved these changes May 9, 2020
Copy link
Contributor

wucke13 left a comment

I left a few comments and questions. Mostly things that I don't understand 😆
In general, the PR looks good to me. Compilation works, including icons for wcm and wf-shell

];

strictDeps = true;
nativeBuildInputs = [ meson ninja pkg-config wayland git ];

This comment has been minimized.

Copy link
@wucke13

wucke13 May 9, 2020

Contributor

Why are git and wayland in the nativeBuildInputs?

This comment has been minimized.

Copy link
@alyssais

alyssais Jun 2, 2020

Author Member

Wayland was there for wayland-scanner. Not sure why git was there. Seems to work fine without it so I’ll drop it.

wayland-protocols wf-config wlroots
];

mesonFlags = [ "--sysconfdir" "/etc" ];

This comment has been minimized.

Copy link
@wucke13

wucke13 May 9, 2020

Contributor

Why is this needed?

This comment has been minimized.

Copy link
@alyssais

alyssais Jun 2, 2020

Author Member

So that Wayfire default configuration files can be placed in /etc. Otherwise people would have no way to set those, because Wayfire would look in its prefix, which can’t be altered.

strictDeps = true;
nativeBuildInputs = [ meson ninja pkg-config ];
buildInputs = [ libevdev libxml2 ];
propagatedBuildInputs = [ glm ];

This comment has been minimized.

Copy link
@wucke13

wucke13 May 9, 2020

Contributor

Out of curiosity, why is propagatedBuildInputs used here for the glm dep?

This comment has been minimized.

Copy link
@alyssais

alyssais Jun 2, 2020

Author Member

Because any package that has wf-config as an input also needs glm as an input. This would otherwise have to be copied every time something had a wf-config input.

];

strictDeps = true;
nativeBuildInputs = [ meson ninja pkg-config wayland git ];

This comment has been minimized.

Copy link
@wucke13

wucke13 May 9, 2020

Contributor

Again, isn't this missing the wrapGAppsHook?
It is not, because of gnome.gtk in buildInputs?


wayfireApplications = wayfireApplications-unwrapped.withPlugins (plugins: [ plugins.wf-shell ]);
inherit (wayfireApplications) wayfire wcm;

This comment has been minimized.

Copy link
@wucke13

wucke13 May 9, 2020

Contributor

I would prefer these to be sorted alphanumerical. Maybe like this:

wayfireApplications = wayfireApplications-unwrapped.withPlugins (plugins: [ plugins.wf-shell ]);
inherit (wayfireApplications) wayfire wcm;
wayfireApplications-unwrapped = callPackage ../applications/window-managers/wayfire/applications.nix { };
wayfirePlugins = callPackage ../applications/window-managers/wayfire/plugins.nix {
  inherit (wayfireApplications-unwrapped) wayfire;
};
wf-config = callPackage ../applications/window-managers/wayfire/wf-config.nix { };

Also: How does this work without defining wcm as a package?

This comment has been minimized.

Copy link
@alyssais

alyssais Jun 2, 2020

Author Member

Also: How does this work without defining wcm as a package?

{ inherit (wayfireApplications) wayfire wcm; }

is equivalent to

{ wayfire = wayfireApplications.wayfire;
  wcm = wayfireApplications.wcm; }

This comment has been minimized.

Copy link
@alyssais

alyssais Jun 2, 2020

Author Member

I’ll resort alphabetically :)

@wucke13
Copy link
Contributor

wucke13 commented May 9, 2020

Huh, on running this standalone I lost the icons on wcm. Maybe wrapGAppsHook is necessary?

Also: We should look at adding some facility for launching dbus to the package. Otherwise things like the gnome pinentry won't work.
ATM I'm using this alias in my shell rc, however that is not such a nice solution:

alias wayfire='[ -z "$DBUS_SESSION_BUS_ADDRESS" ] && export `dbus-launch`; MESA_LOADER_DRIVER_OVERRIDE=i965 wayfire'

ping @alyssais

alyssais added 4 commits May 2, 2020
The top-level "wayfire" attribute is a Wayfire with wf-shell installed
and nothing else.  But wayfireApplications.withPlugins can be used to
create a Wayfire with arbitrary plugins, or no plugins at all.
@alyssais alyssais force-pushed the alyssais:wayfire branch from 432eb67 to 0e6c253 Jun 2, 2020
@alyssais
Copy link
Member Author

alyssais commented Jun 2, 2020

Sorry for being slow with this.

Huh, on running this standalone I lost the icons on wcm. Maybe wrapGAppsHook is necessary?

I tried adding that to both wf-shell and wcm, but still no icons. Any other ideas? Did it work in your PR?

Also: We should look at adding some facility for launching dbus to the package. Otherwise things like the gnome pinentry won't work.

Good idea. I’ll have a look at the sway wrapper and see what goodies we can steal.

@ofborg ofborg bot requested a review from wucke13 Jun 2, 2020
@wucke13
Copy link
Contributor

wucke13 commented Jun 2, 2020

Sorry for being slow with this.

No worries!

I tried adding that to both wf-shell and wcm, but still no icons. Any other ideas? Did it work in your PR?

In my PR all icons but the one on the "Back" icon work.

Good idea. I’ll have a look at the sway wrapper and see what goodies we can steal.

Sweet!

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

2 participants
You can’t perform that action at this time.