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

refactor(nix): home keeping #1380

Merged
merged 5 commits into from
Aug 8, 2024
Merged

Conversation

isabelroses
Copy link
Contributor

changes and why:

  • drop flake-utils: flake-utils was removed from rust-overlay, so pulling it in only to do something that a function could do seems uneeded
  • sync "yazi" to be more in sync with upstream
  • add "?" to "constructed" args since the ordinary callPackage won't pass these args
  • remove the last rec this is following upstreams nixpkgs pattern on this pattern
  • use prev.stdenv.system over final.system since users can disable aliases, we also have no need to use the final specifically either so prev makes more sense
    -shell.nix is now constructed via yazi-unwrapped.nix meaning all dependencies are transfered, reducing some repetion

@isabelroses
Copy link
Contributor Author

isabelroses commented Aug 1, 2024

As such completes this #1376 (comment)

cc: @uncenter

edit: sorry miss read the orginal message

@XYenon
Copy link
Contributor

XYenon commented Aug 1, 2024

Are there any benefits to rewrite a forEachSystem instead of use flake-utils?

flake.nix Outdated
(
final: prev:
let
toolchain = final.rust-bin.stable.latest.default.override { extensions = [ "rust-src" ]; };
Copy link
Contributor

Choose a reason for hiding this comment

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

rust-src is only used by rust-analyzer, this can be moved to shell.nix.

@isabelroses
Copy link
Contributor Author

isabelroses commented Aug 1, 2024

Are there any benefits to rewrite a forEachSystem instead of use flake-utils?

nix clones each repo that you have to the nix store so you end up with an extra storage being taken for such limited use, that can easily be done by any user.

It wouldn't be so bad if rust-overlay we're still tracking it but if its not needed its better to just remove it.

@XYenon
Copy link
Contributor

XYenon commented Aug 1, 2024

Are there any benefits to rewrite a forEachSystem instead of use flake-utils?

nix clones each repo that you have to the nix store so you end up with an extra storage being taken for such limited use, that can easily be done by any user.

It wouldn't be so bad if rust-overlay we're still tracking it but if its not needed its better to just remove it.

I found flake-utils is less than 90KB. I don't think increase the complexity to save 90KB is worth it.

@isabelroses
Copy link
Contributor Author

isabelroses commented Aug 1, 2024

I don't think increase the complexity

Most of that is just from what it looks like but most of what it does you were already doing with:

yazi/flake.nix

Line 29 in 4257b95

pkgs = import nixpkgs { inherit system overlays; };

and

yazi/flake.nix

Lines 17 to 24 in 4257b95

overlays = [
rust-overlay.overlays.default
(final: prev: {
rustToolchain = final.rust-bin.stable.latest.default.override {
extensions = [ "rust-src" ];
};
})
];

Just now most of that complexity is merged into one function, which may make it look worse then it actually is

@XYenon
Copy link
Contributor

XYenon commented Aug 1, 2024

With flake-utils, the flake.nix will be 20 lines shorter.

{
  inputs = {
    nixpkgs.url = "github:NixOS/nixpkgs/nixpkgs-unstable";
    flake-utils.url = "github:numtide/flake-utils";
    rust-overlay = {
      url = "github:oxalica/rust-overlay";
      inputs.nixpkgs.follows = "nixpkgs";
    };
  };

  outputs =
    {
      self,
      nixpkgs,
      flake-utils,
      rust-overlay,
      ...
    }:
    let
      overlays = [
        rust-overlay.overlays.default
        (
          final: prev:
          let
            toolchain = final.rust-bin.stable.latest.default;
          in
          {
            rustPlatform = prev.makeRustPlatform {
              cargo = toolchain;
              rustc = toolchain;
            };
          }
        )
      ];

      rev = self.shortRev or self.dirtyShortRev or "dirty";
      date = self.lastModifiedDate or self.lastModified or "19700101";
      version =
        (builtins.fromTOML (builtins.readFile ./yazi-fm/Cargo.toml)).package.version
        + "pre${builtins.substring 0 8 date}_${rev}";
    in
    flake-utils.lib.eachDefaultSystem (
      system:
      let
        pkgs = import nixpkgs { inherit system overlays; };
      in
      {
        packages = {
          yazi-unwrapped = pkgs.callPackage ./nix/yazi-unwrapped.nix { inherit version rev date; };
          yazi = pkgs.callPackage ./nix/yazi.nix { inherit (self.packages.${system}) yazi-unwrapped; };
          default = self.packages.${system}.yazi;
        };

        devShells = {
          default = pkgs.callPackage ./nix/shell.nix { };
        };

        formatter = pkgs.nixfmt-rfc-style;
      }
    )
    // {
      overlays = {
        default = self.overlays.yazi;
        yazi = _: prev: { inherit (self.packages.${prev.stdenv.system}) yazi yazi-unwrapped; };
      };
    };
}

@isabelroses
Copy link
Contributor Author

you could also make devShells shorter and save 2 lines. But this nix "api" should be mostly (since flakes are experimental in of themselves) unchanging so rewriting it here is not so bad.

 devShells.default = pkgs.callPackage ./nix/shell.nix { };

@XYenon
Copy link
Contributor

XYenon commented Aug 1, 2024

you could also make devShells shorter and save 2 lines.

Make it shorter is not the purpose. I just want to say, reimplement a nix function in a non strongly nix related project is not a good idea.

@isabelroses
Copy link
Contributor Author

I just want to say, reimplement a nix function in a non strongly nix related project is not a good idea.

Normally I would agree but this project has yourself, who works with upstream and @uncenter. Who are both more then capable of understanding and maintaining this.

I also have a bit of a personal grievance with flake-utils, which I won't explain but will point you towards: https://ayats.org/blog/no-flake-utils. As thus I am happy to drop that change though If I cannot change your mind.

@XYenon
Copy link
Contributor

XYenon commented Aug 1, 2024

Normally I would agree but this project has yourself, who works with upstream and @uncenter. Who are both more then capable of understanding and maintaining this.

I also have a bit of a personal grievance with flake-utils, which I won't explain but will point you towards: ayats.org/blog/no-flake-utils. As thus I am happy to drop that change though If I cannot change your mind.

I agree with the opinion of that post. And I'll be happy to maintain a function like this in a nix project, but I don't want to maintain it in every project which uses nix.

Change it to flake-parts is also a great option. 😂

nix/yazi-unwrapped.nix Outdated Show resolved Hide resolved
@sxyazi sxyazi force-pushed the main branch 3 times, most recently from 8f9390b to f865910 Compare August 7, 2024 00:27
@uncenter
Copy link
Contributor

uncenter commented Aug 7, 2024

Anything else here? Would love to get this merged.

@isabelroses
Copy link
Contributor Author

Anything else here? Would love to get this merged.

I think the last thing to do would be to revert back to flake-utils. I do have a way to make the function simpler (at least visually) but as @XYenon said its probably not best.

@isabelroses isabelroses requested a review from XYenon August 7, 2024 09:46
Copy link
Contributor

@uncenter uncenter left a comment

Choose a reason for hiding this comment

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

LGTM

nix/yazi-unwrapped.nix Outdated Show resolved Hide resolved
@isabelroses isabelroses requested a review from XYenon August 8, 2024 11:19
Copy link
Contributor

@XYenon XYenon left a comment

Choose a reason for hiding this comment

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

LGTM :)

Copy link
Owner

@sxyazi sxyazi left a comment

Choose a reason for hiding this comment

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

LGTM, thanks everyone!

I'll merge it now 🙂

@sxyazi sxyazi merged commit e02b030 into sxyazi:main Aug 8, 2024
6 checks passed
@sxyazi
Copy link
Owner

sxyazi commented Aug 8, 2024

It seems the Cachex failed now https://github.com/sxyazi/yazi/actions/runs/10303852646/job/28520903582

@isabelroses please have a look

@isabelroses isabelroses deleted the nix-homekeeping branch August 8, 2024 14:40
@isabelroses
Copy link
Contributor Author

It seems the Cachex failed now https://github.com/sxyazi/yazi/actions/runs/10303852646/job/28520903582

@isabelroses please have a look

My best guess here is that rust_overlay is now using rust 1.80 and its unhappy with cargo-c's code thus is generating an error. We could test by rolling back a few versions.

@sxyazi
Copy link
Owner

sxyazi commented Aug 8, 2024

Should I revert this PR and you can submit a new one?

@isabelroses
Copy link
Contributor Author

Should I revert this PR and you can submit a new one?

I can create a Pr in a second pinning the version, when i find a successful build.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants