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

gnome: switch to makeScopeWithSplicing #154751

Closed
wants to merge 1 commit into from
Closed

Conversation

NickCao
Copy link
Member

@NickCao NickCao commented Jan 12, 2022

Motivation for this change

makeScope breaks splicing thus makes cross compilation hard in package sets like gnome, by switching to makeScopeWithSplicing, the situation can be dramatically improved.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added the 6.topic: GNOME GNOME desktop environment and its underlying platform label Jan 12, 2022
@NickCao NickCao added the 6.topic: cross-compilation Building packages on a different platform than they will be used on label Jan 12, 2022
@NickCao
Copy link
Member Author

NickCao commented Jan 12, 2022

Use gnome-power-manager as an example, it won't cross build without this pr applied:

gnome-power-manager> /nix/store/5h3ziqff9fkn0jvqmvl3bssfd5l0dg5n-glib-riscv64-unknown-linux-gnu-2.70.2-dev/bin/glib-compile-resources: line 1: syntax error: unexpected "("

And it is obvious that glib from hostPlatform instead of buildPlatform was used, despite that it is in nativeBuildInputs.
With this pr and #148618, #151184 and NickCao@01192ff applied, it can be successfully crossed from x86_64-linux to riscv64-linux, the same applies to other packages in the gnome package set.

@NickCao NickCao marked this pull request as ready for review January 12, 2022 12:36
@NickCao
Copy link
Member Author

NickCao commented Jan 12, 2022

cc @Mindavi as I think you might also be interested.

@Ericson2314
Copy link
Member

Thanks for doing this! Always make me very happy to see others fixing cross compilation issues.

@NickCao
Copy link
Member Author

NickCao commented Jan 16, 2022

After the removal of aliases, cross evaluation is failing on throw. While they are outside the scope, they seem to be brought back in by the spliced package sets.

@NickCao NickCao marked this pull request as draft January 16, 2022 02:14
@jtojnar
Copy link
Member

jtojnar commented Jan 16, 2022

Can we detect that the attrset is being spliced and hide the aliases in that case as well?

@jtojnar
Copy link
Member

jtojnar commented Jan 16, 2022

This ugly patch seems to work:

--- a/pkgs/desktops/gnome/default.nix
+++ b/pkgs/desktops/gnome/default.nix
@@ -1,12 +1,12 @@
-{ config, pkgs, lib, splicePackages, newScope
+{ config, pkgs, lib, splicePackages, newScope, isSpliced ? false
 , pkgsBuildBuild, pkgsBuildHost, pkgsBuildTarget, pkgsHostHost, pkgsTargetTarget }:
 
 lib.makeScopeWithSplicing splicePackages newScope {
-  selfBuildBuild = pkgsBuildBuild.gnome;
-  selfBuildHost = pkgsBuildHost.gnome;
-  selfBuildTarget = pkgsBuildTarget.gnome;
-  selfHostHost = pkgsHostHost.gnome;
-  selfTargetTarget = pkgsTargetTarget.gnome or {};
+  selfBuildBuild = pkgsBuildBuild.gnome.override { isSpliced = true; };
+  selfBuildHost = pkgsBuildHost.gnome.override { isSpliced = true; };
+  selfBuildTarget = pkgsBuildTarget.gnome.override { isSpliced = true; };
+  selfHostHost = pkgsHostHost.gnome.override { isSpliced = true; };
+  selfTargetTarget = if pkgsTargetTarget ? gnome then pkgsTargetTarget.gnome.override { isSpliced = true; } else { };
 } (self: { }) (spliced0: { })
 (self: let inherit (self) callPackage; in {
   updateScript = callPackage ./update.nix { };
@@ -288,7 +288,7 @@ lib.makeScopeWithSplicing splicePackages newScope {
   gnome-autoar = callPackage ./misc/gnome-autoar { };
 
   gnome-packagekit = callPackage ./misc/gnome-packagekit { };
-}) // lib.optionalAttrs (config.allowAliases or true) {
+}) // lib.optionalAttrs (config.allowAliases or true && !isSpliced) {
 #### Legacy aliases. They need to be outside the scope or they will shadow the attributes from parent scope.
 
   bijiben = throw "The ‘gnome.bijiben’ alias was removed on 2022-01-13. Please use ‘gnome.gnome-notes’ directly."; # added 2018-09-26

@NickCao
Copy link
Member Author

NickCao commented Jan 16, 2022

This seems to be a viable fix, really ugly though. Let's keep this pr as a draft until we really figure out the proper way to deal with the conflict between makeScope and splicing.

@jtojnar
Copy link
Member

jtojnar commented Jan 16, 2022

Well, applying the aliases on top of the scope is really ugly too. And so is the splicing code.
Maybe the best option is really just biting the bullet and moving everything under gnome into the top-level.

@Ericson2314
Copy link
Member

Hmm as jank as splicing is, I am surprised the throwing attributes would effect the other attributes.

@jtojnar
Copy link
Member

jtojnar commented Jan 16, 2022

The issue is that the throwing attributes shadow attributes from pkgs, which are used by other packages in the scope.

We are intentionally moving them outside the scope to avoid the shadowing while leaving them accessible as attributes under gnome. But that will fail when the attributes are actually pulled through pkgs*.gnome since those include the throwers.

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 31, 2022
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Aug 11, 2022
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 19, 2024
@NickCao
Copy link
Member Author

NickCao commented Sep 9, 2024

The gnome scope no longer exists.

@NickCao NickCao closed this Sep 9, 2024
@NickCao NickCao deleted the gnome branch September 10, 2024 00:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: cross-compilation Building packages on a different platform than they will be used on 6.topic: GNOME GNOME desktop environment and its underlying platform 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants