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

mate.mate-tweak: init at 20.10.0 #85955

Merged
merged 1 commit into from Nov 2, 2020
Merged

Conversation

luc65r
Copy link
Contributor

@luc65r luc65r commented Apr 24, 2020

Motivation for this change

Add mate-tweak, because it's missing.

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.

pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
pkgs/tools/misc/mate-tweak/default.nix Outdated Show resolved Hide resolved
pkgs/tools/misc/mate-tweak/default.nix Outdated Show resolved Hide resolved
pkgs/tools/misc/mate-tweak/default.nix Outdated Show resolved Hide resolved
pkgs/tools/misc/mate-tweak/default.nix Outdated Show resolved Hide resolved
pkgs/tools/misc/mate-tweak/default.nix Outdated Show resolved Hide resolved
pkgs/tools/misc/mate-tweak/default.nix Outdated Show resolved Hide resolved
pkgs/tools/misc/mate-tweak/default.nix Outdated Show resolved Hide resolved
Comment on lines 52 to 59
postPatch = ''
# mate-tweak hardcodes absolute paths everywhere. Nuke from orbit.
find . -type f -exec sed -i \
-e s,/usr/lib/mate-tweak,$out/lib/mate-tweak,g \
{} +
'';
Copy link
Contributor

Choose a reason for hiding this comment

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

there's still a ton more hardcoded paths in this project.
I'm also seeing code here https://github.com/ubuntu-mate/mate-tweak/blob/2ccffffdff2b2f831cd05f787e37c0f297c1422f/util/mate-tweak-helper#L30, that will probably be entirely broken on nixos

Copy link
Member

Choose a reason for hiding this comment

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

I've talked with @luc65r and the result was that it would be a lot of work to actually fix every usecase and that making it work on mate properly should be enough for shipping

@worldofpeace
Copy link
Contributor

OMG sooo mannyyyyyyy hardcoded FHS paths https://github.com/ubuntu-mate/mate-tweak/blob/927b2ede5ec2042238f6da229e60a14ee7c124cf/mate-tweak, I have no idea to figure out if things will work properly. It for sure won't ever be hermetic outside having the mate desktop enabled in NixOS.

@luc65r
Copy link
Contributor Author

luc65r commented Apr 26, 2020

I tested it on a VM, and most of the settings work, except the panel layout.
Also, when it asks for the user's password, it asks it 5 to 10 times.
There are optional dependencies, but as I'm still not great at packaging, I decided to not bother with them.

@worldofpeace
Copy link
Contributor

I tested it on a VM, and most of the settings work, except the panel layout.
Also, when it asks for the user's password, it asks it 5 to 10 times.
There are optional dependencies, but as I'm still not great at packaging, I decided to not bother with them.

When it asks for user password, is it trying to get elevated privileges to modify the system?
That as much should be prevented on NixOS.

@luc65r
Copy link
Contributor Author

luc65r commented Apr 26, 2020

Yes, it does.

@worldofpeace worldofpeace changed the title mate-tweak: init at 20.04.0 mate.mate-tweak: init at 20.04.0 Apr 26, 2020
@luc65r luc65r force-pushed the pkg/mate-tweak branch 3 times, most recently from aabac93 to 134a676 Compare April 26, 2020 19:47
@luc65r
Copy link
Contributor Author

luc65r commented Apr 26, 2020

There is no longer usr in any of the output files.

@stale
Copy link

stale bot commented Oct 24, 2020

Hello, I'm a bot and I thank you in the name of the community for your contributions.

Nixpkgs is a busy repository, and unfortunately sometimes PRs get left behind for too long. Nevertheless, we'd like to help committers reach the PRs that are still important. This PR has had no activity for 180 days, and so I marked it as stale, but you can rest assured it will never be closed by a non-human.

If this is still important to you and you'd like to remove the stale label, we ask that you leave a comment. Your comment can be as simple as "still important to me". But there's a bit more you can do:

If you received an approval by an unprivileged maintainer and you are just waiting for a merge, you can @ mention someone with merge permissions and ask them to help. You might be able to find someone relevant by using Git blame on the relevant files, or via GitHub's web interface. You can see if someone's a member of the nixpkgs-committers team, by hovering with the mouse over their username on the web interface, or by searching them directly on the list.

If your PR wasn't reviewed at all, it might help to find someone who's perhaps a user of the package or module you are changing, or alternatively, ask once more for a review by the maintainer of the package/module this is about. If you don't know any, you can use Git blame on the relevant files, or GitHub's web interface to find someone who touched the relevant files in the past.

If your PR has had reviews and nevertheless got stale, make sure you've responded to all of the reviewer's requests / questions. Usually when PR authors show responsibility and dedication, reviewers (privileged or not) show dedication as well. If you've pushed a change, it's possible the reviewer wasn't notified about your push via email, so you can always officially request them for a review, or just @ mention them and say you've addressed their comments.

Lastly, you can always ask for help at our Discourse Forum, or more specifically, at this thread or at #nixos' IRC channel.

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Oct 24, 2020
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Oct 29, 2020
@luc65r luc65r force-pushed the pkg/mate-tweak branch 2 times, most recently from ac8863e to 94d2661 Compare October 29, 2020 11:06
@luc65r luc65r changed the title mate.mate-tweak: init at 20.04.0 mate.mate-tweak: init at 20.10.0 Oct 29, 2020
Copy link
Member

@mkg20001 mkg20001 left a comment

Choose a reason for hiding this comment

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

LGTM

@mkg20001
Copy link
Member

@worldofpeace Can we get this in?

@worldofpeace
Copy link
Contributor

@mkg20001 I can check in on #85955 (comment)

@worldofpeace
Copy link
Contributor

Still crashes with

(mate-tweak:29513): GLib-GIO-ERROR **: 17:41:44.100: Settings schema 'org.mate.session.required-components' is not installed

in a pure environment. Can we add mate-session to buildInputs?

@luc65r
Copy link
Contributor Author

luc65r commented Oct 30, 2020

@worldofpeace I can't reproduce this behaviour. I added mate-session-manager hoping it will solve this problem.

@mkg20001
Copy link
Member

@luc65r if you're testing on mate than usually the required files are provided via env variables inherited from the session, if you're testing on non-mate that's no longer the case so they're missing

@luc65r
Copy link
Contributor Author

luc65r commented Oct 30, 2020

@mkg20001 I tried on mate in a VM, and on i3wm where I believe I have nothing installed that is related to mate.
Anyway, I don't see why someone who is not using mate would use mate-tweak.

@mkg20001
Copy link
Member

@luc65r Ok, then it's a mystery.

nixOS ppl aren't fans of implicit dependencies, everything has to be explicit. that's mostly why.

@worldofpeace
Copy link
Contributor

@mkg20001 I tried on mate in a VM, and on i3wm where I believe I have nothing installed that is related to mate.
Anyway, I don't see why someone who is not using mate would use mate-tweak.

It's not to cater to people not using mate, it is simply improper packaging as @mkg20001 mentioned. In nixos we don't have FHS /usr so gsettings-schemas are not installed globally. If you don't add the packages that have them in buildInputs for wrapGAppsHook everything will be broken.

I've actually just investigated why this happens, https://github.com/ubuntu-mate/mate-tweak/blob/f001d41fca81d80a5b9ca27a113696634a16c290/mate-tweak#L992, I'm using plank so I trigger this code. Several other places need this key so we need to add it to buildInputs. I would check if other code is needing a schema that isn't in buildInputs by checking self.get_string( in the code.

Copy link
Member

@mkg20001 mkg20001 left a comment

Choose a reason for hiding this comment

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

LGTM

@mkg20001 mkg20001 merged commit f0d05c4 into NixOS:master Nov 2, 2020
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