Skip to content

buildFHSChrootEnv: link gsettings-schemas to FHS location#83705

Closed
worldofpeace wants to merge 3 commits intoNixOS:masterfrom
worldofpeace:fhs-gsettings
Closed

buildFHSChrootEnv: link gsettings-schemas to FHS location#83705
worldofpeace wants to merge 3 commits intoNixOS:masterfrom
worldofpeace:fhs-gsettings

Conversation

@worldofpeace
Copy link
Contributor

@worldofpeace worldofpeace commented Mar 29, 2020 •
edited
Loading

We shouldn't need to use wrapGAppsHook in expressions
that use this builder. The code to do this is from #54083.

Motivation for this change

#83410 ssb-patchwork should now work without any wrapping.

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.

@worldofpeace worldofpeace mentioned this pull request Mar 29, 2020
Closed
10 tasks
@worldofpeace
Copy link
Contributor Author

worldofpeace commented Mar 29, 2020

@hedning I found a use for your code ✨

@flokli
Copy link
Member

flokli commented Mar 29, 2020

@worldofpeace awesome! Can you add a line somewhere in doc/builders/special/fhs-environments.xml explaining that this is doing this now as well?

@worldofpeace
Copy link
Contributor Author

worldofpeace commented Mar 29, 2020

@worldofpeace awesome! Can you add a line somewhere in doc/builders/special/fhs-environments.xml explaining that this is doing this now as well?

Didn't know there was docs. Will do.

jtojnar
jtojnar reviewed Mar 29, 2020
View reviewed changes
@flokli
Copy link
Member

flokli commented Mar 29, 2020 •
edited
Loading

btw, I checked if anything using buildFHSUserEnv is also using wrapGAppsHook - it isn't.

@ofborg ofborg bot added the 8.has: documentation This PR adds or changes documentation label Mar 29, 2020
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. labels Mar 29, 2020
@worldofpeace
Copy link
Contributor Author

worldofpeace commented Mar 29, 2020

Meh, this isn't right. I will finish this tomorrow.

@picnoir
Copy link
Member

picnoir commented Mar 30, 2020

Maybe here?

nixpkgs/pkgs/build-support/appimage/default.nix

Line 39 in ecfb2b4

defaultFhsEnvArgs = {

@worldofpeace
Copy link
Contributor Author

worldofpeace commented Mar 30, 2020

Maybe here?

nixpkgs/pkgs/build-support/appimage/default.nix

Line 39 in ecfb2b4

defaultFhsEnvArgs = {

I believe the appimage build support code, at its lowest level, uses this builder.

@ghost
Copy link

ghost commented Apr 10, 2020

Missing schemas also crash riot-desktop(electron), its not oly isolated to ssb-patchwork.

@worldofpeace
Copy link
Contributor Author

worldofpeace commented Apr 10, 2020

Missing schemas also crash riot-desktop(electron), its not oly isolated to ssb-patchwork.

riot-desktop doesn't use this builder at all, and I've already fixed that fe6addb

@nixos-discourse
Copy link

nixos-discourse commented Apr 11, 2020

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/declarative-wrappers-another-idea/6661/1

@cyplo
Copy link
Contributor

cyplo commented May 24, 2020

heya folks - awesome work :) Is there something more we need to do to get this one merged ? :)

jtojnar
jtojnar reviewed May 24, 2020
View reviewed changes
pkgs/build-support/build-fhs-userenv/env.nix
if [[ -d $out/share/gsettings-schemas/ ]]; then
# Create the standard schemas directory if needed
if [[ -L $out/share/glib-2.0 ]]; then
rm -rf $out/share/glib-2.0
Copy link
Member

@jtojnar jtojnar May 24, 2020

Choose a reason for hiding this comment

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

I am still not totally comfortable with removing the link. At the moment, I cannot think of a case where it would be destructive but conditions change.

Could we try to make the tree writeable like lndir does?

Copy link
Contributor

@asymmetric asymmetric Jun 17, 2020

Choose a reason for hiding this comment

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

@jtojnar could you point me to the lndir package you're referring to?

Copy link
Member

@jtojnar jtojnar Jun 17, 2020

Choose a reason for hiding this comment

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

It is in xorg.lndir.

Copy link
Contributor

@asymmetric asymmetric Jun 17, 2020 •
edited by jtojnar
Loading

Choose a reason for hiding this comment

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

@jtojnar I don't see anything relevant here

Copy link
Member

@jtojnar jtojnar Jun 17, 2020

Choose a reason for hiding this comment

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

I meant the lndir program, not the expression.

Copy link
Member

@Artturin Artturin Feb 24, 2022

Choose a reason for hiding this comment

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

#161739

@picnoir picnoir mentioned this pull request Jun 16, 2020
Open
@picnoir picnoir mentioned this pull request Nov 23, 2020
Closed
@mkf
Copy link
Contributor

mkf commented Nov 23, 2020

May I mention (in case it may help anybody) that i just tried running ssb-patchwork from the nix-shell ran by nixpkgs-review for this PR and ssb-patchwork crashed just the same when trying to choose an avatar.

@SuperSandro2000 SuperSandro2000 added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 18, 2021
@stale
Copy link

stale bot commented Jul 21, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 21, 2021
@picnoir
Copy link
Member

picnoir commented Jul 26, 2021

Still relevant to me.

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 26, 2021
@Artturin Artturin mentioned this pull request Feb 24, 2022
Merged
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@asymmetric asymmetric asymmetric left review comments

@jtojnar jtojnar jtojnar left review comments

@Artturin Artturin Artturin left review comments

Assignees

No one assigned

Labels

2.status: merge conflict This PR has merge conflicts with the target branch 8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@worldofpeace @flokli @picnoir @nixos-discourse @cyplo @mkf @asymmetric @jtojnar @Artturin @SuperSandro2000