Skip to content

Conversation

eclairevoyant
Copy link
Contributor

@eclairevoyant eclairevoyant commented Jul 29, 2024

Description of changes

Reformat with nixfmt-rfc-style, and declare some options:

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 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
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added the 6.topic: stdenv Standard environment label Jul 29, 2024
@@ -140,9 +182,26 @@ let
feature = "build packages with ROCm support by default";
};

packageOverrides = mkOption {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We might also want to deprecate this.

@@ -140,9 +182,26 @@ let
feature = "build packages with ROCm support by default";
};

packageOverrides = mkOption {
type = types.functionTo types.raw;
Copy link
Contributor Author

@eclairevoyant eclairevoyant Jul 29, 2024

Choose a reason for hiding this comment

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

Should this be functionTo (lazyAttrsOf raw) instead of functionTo raw?

@eclairevoyant eclairevoyant changed the title pkgs/top-level/config: format, declare options pkgs/top-level/config.nix: nixfmt, declare options Jul 29, 2024
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels Jul 29, 2024
@eclairevoyant eclairevoyant marked this pull request as ready for review July 31, 2024 14:00
Copy link
Contributor

@philiptaron philiptaron left a comment

Choose a reason for hiding this comment

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

I took a look at the updated nixpkgs-manual as well. Looks good.

@philiptaron
Copy link
Contributor

I plan on merging this on Monday unless I hear from others about Issues.

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one person. label Aug 4, 2024
@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one person. label Aug 5, 2024
Copy link
Contributor

@philiptaron philiptaron left a comment

Choose a reason for hiding this comment

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

Did a final nixpkgs-manual build, diff'd it with master, looks good 👍🏻

@philiptaron philiptaron merged commit 42d0012 into NixOS:master Aug 5, 2024
21 checks passed
@spiage
Copy link

spiage commented Aug 5, 2024

@infinisil Hi! this breaks master's build
42d0012#commitcomment-145046128

@philiptaron
Copy link
Contributor

@spiage could you provide a link to your system's flake.nix? I don't see an "a7" in https://github.com/spiage/nixos-config/blob/main/flake.nix

@spiage
Copy link

spiage commented Aug 5, 2024

well, I was not ready to open this machine's config, but OK, give me 2 minutes

@eclairevoyant
Copy link
Contributor Author

eclairevoyant commented Aug 5, 2024

It's reproducible by adding an insecure package to a nixos config. Maybe we should revert for now?

{
  inputs = {
    nixpkgs.url = "github:nixos/nixpkgs";
  };

  outputs =
    { nixpkgs, ... }:
    {
      nixosConfigurations.test = nixpkgs.lib.nixosSystem {
        modules = [
          (
            { modulesPath, pkgs, ... }:
            {
              imports = [ "${modulesPath}/profiles/minimal.nix" ];

              boot.loader.grub.enable = false;
              fileSystems."/".device = "nodev";
              nixpkgs.hostPlatform = "x86_64-linux";
              system.stateVersion = "24.05";

              nixpkgs.config = {
                warnUndeclaredOptions = true;
                #permittedInsecurePackages = [ "electron" ];
                #allowInsecurePredicate = _: false;
              };

              environment.systemPackages = [ pkgs.electron_27-bin ];

              # custom config here
            }
          )
        ];
      };
    };
}

@spiage
Copy link

spiage commented Aug 5, 2024

I'm using yandex-browser
and it have this lines

@philiptaron
Copy link
Contributor

It's reproducible by adding an insecure package to a nixos config. Maybe we should revert for now?

Yeah, unless the fix is super simple. FWIW, I have an unfree package in my flake and no issues.

@spiage
Copy link

spiage commented Aug 5, 2024

if it is my local problem I can use any walkthrough if you give me =)

@eclairevoyant
Copy link
Contributor Author

Basically I didn't notice that the config.foo or ... constructions would no longer work, and it would either have to be replaced with a check on options.foo.isDefined or by explicitly setting the default

@philiptaron
Copy link
Contributor

Basically I didn't notice that the config.foo or ... constructions would no longer work, and it would either have to be replaced with a check on options.foo.isDefined or by explicitly setting the default

That's pretty hefty search-and-replace, and definitely a breaking change for external users. Setting the default is the least impactful option, as I see it, especially if there's a common and obvious default. Do you agree?

@spiage
Copy link

spiage commented Aug 5, 2024

@spiage could you provide a link to your system's flake.nix? I don't see an "a7" in https://github.com/spiage/nixos-config/blob/main/flake.nix

I am very ashamed to show it, but here is the config
https://github.com/spiage/nixos-config-a7

@eclairevoyant
Copy link
Contributor Author

Basically I didn't notice that the config.foo or ... constructions would no longer work, and it would either have to be replaced with a check on options.foo.isDefined or by explicitly setting the default

That's pretty hefty search-and-replace, and definitely a breaking change for external users. Setting the default is the least impactful option, as I see it, especially if there's a common and obvious default. Do you agree?

Yes, I think setting default would be safer, but I don't have enough time to test right now. I'm okay with reverting for now and making a new PR with some test cases added.

@spiage
Copy link

spiage commented Aug 5, 2024

I'm ready to help with tests if you need it =)

@philiptaron
Copy link
Contributor

Thanks for the super-fast report @spiage! I'm going to merge @eclairevoyant's revert and you'll be on your way.

@eclairevoyant eclairevoyant deleted the nixpkgs-declare-options branch August 5, 2024 16:46
@spiage
Copy link

spiage commented Aug 5, 2024

@spiage
Copy link

spiage commented Aug 5, 2024

It's ok after revert

Version 15449 -> 15450:
  nixos-system-a7: 24.11.20240805.6be7a53 → 24.11.20240805.fb7d848
  source: +14.1 KiB

@philiptaron
Copy link
Contributor

@eclairevoyant, I'd love to get this PR rolling again. What do you see as the set of steps to get it in without causing a hullabaloo?

@eclairevoyant
Copy link
Contributor Author

eclairevoyant commented Sep 5, 2024

I need to find free time to write some tests 😩

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/community-team-updates/56458/11

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: stdenv Standard environment 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

warnUndeclaredOptions warns about packageOverrides
5 participants