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

firefox: add gnome-theme testbed #879

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

brckd
Copy link
Contributor

@brckd brckd commented Feb 18, 2025

Adds a separate testbed for Firefox with firefoxGnomeTheme.enable = true as per
#702 (comment).

@brckd
Copy link
Contributor Author

brckd commented Feb 18, 2025

I just tested it and it works, but it has some weird artifacts.
Screencast From 2025-02-18 18-36-18.webm
Screenshot from Screencast From 2025-02-18 18-36-18 webm - 1
This only happens in the testbed, so I wouldn't be too concerned about this.

Copy link
Collaborator

@trueNAHO trueNAHO left a comment

Choose a reason for hiding this comment

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

Considering hyphens are not allowed in testbed names, should we rename the testbed to gnome_theme? This naming convention scales better, if we add longer testbed names in the future. @danth, what do you think?

@danth
Copy link
Owner

danth commented Feb 19, 2025

We should find a better solution for naming testbeds so that hyphens are allowed, as hyphens are pretty standard for this sort of thing in other Nix projects. Potentially we could move to a nested structure, although that may be incompatible with certain nix commands.

For the purposes of this PR I would just leave it as gnometheme.

@brckd
Copy link
Contributor Author

brckd commented Feb 19, 2025

Some module names also include hyphens, so I'd consider a different separator like . when running a testbed. So you'd run nix run .#testbed.module.testbed.polarity. We could still replace that with - in the derivation name like testbed-module-testbed-polarity because derivation names usually don't carry semantic meaning.

@brckd
Copy link
Contributor Author

brckd commented Feb 19, 2025

I don't really get why this testbed is failing, because it's pretty much identical to the other firefox testbed.

@danth
Copy link
Owner

danth commented Feb 19, 2025

The nested structure I mentioned would work with .s as you described - only issue is things like nix flake show might not list the packages correctly

@danth
Copy link
Owner

danth commented Feb 19, 2025

I don't really get why this testbed is failing, because it's pretty much identical to the other firefox testbed.

This will be due to the use of builtins.readFile in the GNOME theme part of the module. We could try replacing it with an @import if that works

@brckd
Copy link
Contributor Author

brckd commented Feb 19, 2025

I don't really get why this testbed is failing, because it's pretty much identical to the other firefox testbed.

This will be due to the use of builtins.readFile in the GNOME theme part of the module. We could try replacing it with an @import if that works

Yea, that should work. Thanks for pointing it out! So I assume that builtins.readFile isn't declarative enough for derivations, but builtins.import is.

@brckd
Copy link
Contributor Author

brckd commented Feb 19, 2025

Ok, it seems like import is not allowed either.

@brckd
Copy link
Contributor Author

brckd commented Feb 19, 2025

According to https://nix.dev/manual/nix/2.25/language/import-from-derivation, this happens when a derivation depends on another derivation in any way. It's actually just a performance measure, so I wonder if we can somehow turn it off. The allow-import-from-derivation is enabled by default, so perhaps Determinate Nix turned it off.

@trueNAHO
Copy link
Collaborator

Some module names also include hyphens

True.

so I'd consider a different separator like . when running a testbed. So you'd run nix run .#testbed.module.testbed.polarity. We could still replace that with - in the derivation name like testbed-module-testbed-polarity because derivation names usually don't carry semantic meaning.

The . separator seems very convenient.

The nested structure I mentioned would work with .s as you described - only issue is things like nix flake show might not list the packages correctly

In that case, derivation names need to be quoted:

nix run '.#"testbed.module-name.dark"'

Since this is fairly annoying to type, we could chose another separator, like ::

nix run .#testbed:module-name:dark

According to https://nix.dev/manual/nix/2.25/language/import-from-derivation, this happens when a derivation depends on another derivation in any way. It's actually just a performance measure, so I wonder if we can somehow turn it off. The allow-import-from-derivation is enabled by default, so perhaps Determinate Nix turned it off.

IFD is disabled inside the CI since commit d8289c3 ("ci: disable IFD (#855)").

@danth
Copy link
Owner

danth commented Feb 21, 2025

What about this?

 userChrome =
  let template = config.lib.stylix.colors {
    template = ./userChrome.mustache;
    extension = "css";
  };
  in ''
    @import "${template}";
  '';

IFD is intentionally disabled in the CI as some systems, such as Hydra, don't allow it by default, so we try to avoid IFD wherever possible so that user's configurations are compatible with that (and also the performance improvements as you mentioned).

@trueNAHO
Copy link
Collaborator

trueNAHO commented Feb 21, 2025

Some module names also include hyphens

True.

so I'd consider a different separator like . when running a testbed. So you'd run nix run .#testbed.module.testbed.polarity. We could still replace that with - in the derivation name like testbed-module-testbed-polarity because derivation names usually don't carry semantic meaning.

The . separator seems very convenient.

The nested structure I mentioned would work with .s as you described - only issue is things like nix flake show might not list the packages correctly

In that case, derivation names need to be quoted:

nix run '.#"testbed.module-name.dark"'

Since this is fairly annoying to type, we could chose another separator, like ::

nix run .#testbed:module-name:dark

I implemented this in #887.

@danth
Copy link
Owner

danth commented Feb 21, 2025

In that case, derivation names need to be quoted

I meant that we use attribute sets inside attribute sets, like

{
  gnome = {
    default = {
      light = «derivation»;
      dark = «derivation»;
    };
  };
}

This would not require quoting.

However I can confirm this is not supported by nix flake show and results in an error, unless we move it to legacyPackages.

@brckd
Copy link
Contributor Author

brckd commented Feb 21, 2025

In my opinion legacyPackages would indeed be the solution here. There is nothing legacy about legacyPackages and it's simply intended for nested packages like this one. I think this is also a common pattern.

@trueNAHO
Copy link
Collaborator

trueNAHO commented Feb 21, 2025

In my opinion legacyPackages would indeed be the solution here. There is nothing legacy about legacyPackages and it's simply intended for nested packages like this one. I think this is also a common pattern.

Can legacyPackages be added to the checks output? If not, then we have to flatten the legacyPackages structure when adding it to checks, which negates the problem legacyPackages is trying to solve. Instead, #887 solves the flat structure itself.

Adds a separate testbed for Firefox with `firefoxGnomeTheme.enable =
true` as per
danth#702 (comment).
@brckd brckd force-pushed the firefox/add-gnome-theme-testbed branch 2 times, most recently from d1a382a to da580fd Compare February 25, 2025 00:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants