Skip to content

The Nix pipelines #1573

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

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

The Nix pipelines #1573

wants to merge 20 commits into from

Conversation

Sk7Str1p3
Copy link
Contributor

@Sk7Str1p3 Sk7Str1p3 commented May 4, 2025

This pull request supersedes #1549 with much better approaches.
Features:

  • Development shell
  • Derivation to build from latest git
  • Automatic checks (formatting, clippy, audit, nextest)
  • Download ready crates from cachix/garnix
  • Automatic flake.lock update

This can also be used in CI for much more powerful checks

Sk7Str1p3 added 6 commits May 4, 2025 19:04
> add nix tools to devShell (rust-required stuff provided by default)

> add build dependencies

> add build profiles
@Sk7Str1p3
Copy link
Contributor Author

By the way, as I am newbie to rust ecosystem, I suppose I did some bad configuration in toml configs. If you think so, let me know.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't work in the sandbox

What happens in the sandbox? Does it fail to send HTTP requests to crates.io?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did not checked (this is crane generated defaults) but I suppose yes it fails

Copy link
Collaborator

Choose a reason for hiding this comment

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

Got it. Makes sense if this file was generated by some tool.

But I don't think we'd want to disable audits for yanked crates just because a dev tool doesn't work well with it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Setting up TOML formatting sounds good to me, but TBH I think this can go in a separate PR. That way we can get TOML formatting merged in faster while we still discuss flake.

flake.lock Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll have to research flake later, but could you discuss the pros of tracking the lock file in the repository?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same pros of tracking cargo.lock in repository - to be sure the shell will be SAME, and to be sure it is going to behave same way anywhere.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I suppose the question is: do we need that amount of consistency? Or can we trust that the requirements and constraints in flake.nix are most likely good enough?

Continuing with the comparison to Cargo.lock, there are many times where you don't commit that lock file, because you don't need to use exact versions. This is common when authoring libraries, for example.

Choose a reason for hiding this comment

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

Not including it would null most of nix's benefits. I have yet to find a single nix flake without its respective lockfile attached.

Just my 2cts here. :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see, that makes sense. But I believe having to regularly update the lock file was part of the reason that the initial attempt was reverted (#1549 (comment)).

I'm guessing that these benefits are being able to create a reproducible environment? IMO that's not always the highest priority -- our devcontainer config isn't that strict, for example (actually I think it's a bit too strict right now). Sometimes ease of setup, with the assumption that a well-functioning environment will be built, is enough.

Copy link
Contributor Author

@Sk7Str1p3 Sk7Str1p3 Jun 5, 2025

Choose a reason for hiding this comment

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

That's why I added workflow updating flake.lock regularly (exactly, once in a week).

Although now I realized I didn't check it yet and don't really know how do I do that 😅.

I'm doing some research right now.

Also, (I didn't checked yet, but) I believe nix would refuse to work with flake.lock in .gitignore

@doprz
Copy link

doprz commented May 8, 2025

I would also recommend using nix-direnv

A faster, persistent implementation of direnv's use_nix and use_flake, to replace the built-in one.

@Sk7Str1p3
Copy link
Contributor Author

I think I should recreate flake from scratch. Now I worked much more with crane and I can say it can be better. Much better

@Sk7Str1p3
Copy link
Contributor Author

I believe we should include clippy, deny and audit checks in our CI. Also it is possible to build CI with nix (this should speed up checks because of dependency caching)

@Sk7Str1p3
Copy link
Contributor Author

Please take a look at taplo.toml, I suppose you would not like some features enabled

taplo.toml Outdated
reorder_inline_tables = true
reorder_keys = true
#
crlf = true
Copy link
Collaborator

@spenserblack spenserblack Jun 3, 2025

Choose a reason for hiding this comment

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

Definitely not CRLF for formatting.

We do try to alphabetize, so that makes sense. Other than that, I'll leave preferences up to @o2sh to decide.

I'll probably cherry-pick the taplo configuration, since it's not dependent on any sort of Nix configuration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Btw @o2sh this is formatted Cargo.toml

[workspace.package]
  authors    = [ "o2sh <[email protected]>" ]
  edition    = "2021"
  license    = "MIT"
  repository = "https://github.com/o2sh/onefetch"
  version    = "2.23.1"

[workspace]
  members = [ "ascii", "image", "manifest" ]
  [workspace.dependencies]
    anyhow = "1.0"
    clap = { version = "4.5.26", features = [ "derive" ] }
    image = { version = "0.25.5", default-features = false, features = [
      "color_quant",
      "jpeg",
      "png",
      "webp",
    ] }
    owo-colors = "4.1.0"
    strum = { version = "0.26.3", features = [ "derive" ] }

[package]
  authors.workspace    = true
  categories           = [ "command-line-utilities" ]
  description          = "Command-line Git information tool"
  edition.workspace    = true
  exclude              = [ "docs/vercel/*" ]
  homepage             = "https://onefetch.dev"
  keywords             = [ "cli", "git", "terminal" ]
  license.workspace    = true
  name                 = "onefetch"
  repository.workspace = true
  rust-version         = "1.81.0"
  version.workspace    = true

[dependencies]
  anyhow.workspace  = true
  askalono          = "0.5.0"
  byte-unit         = "5.1.6"
  clap.workspace    = true
  clap_complete     = "4.5.42"
  crossbeam-channel = "0.5.14"
  #
  gix = { version = "0.70.0", default-features = false, features = [
    "blob-diff",
    "index",
    "mailmap",
    "max-performance-safe",
    "status",
  ] }
  #
  gix-features         = { version = "0.40.0", features = [ "zlib-ng" ] }
  globset              = "0.4.15"
  human-panic          = "2.0.2"
  image.workspace      = true
  num-format           = "0.4.4"
  onefetch-ascii       = { path = "ascii", version = "2.19.0" }
  onefetch-image       = { path = "image", version = "2.19.0" }
  onefetch-manifest    = { path = "manifest", version = "2.19.0" }
  owo-colors.workspace = true
  regex                = "1.11.1"
  serde                = "1.0"
  serde_json           = "1.0"
  serde_yaml           = "0.9.34"
  # TODO With the new value parsers, we're really close to being able to eliminate
  # the strum dependency
  strum.workspace = true
  time            = { version = "0.3.37", features = [ "formatting" ] }
  time-humanize   = { version = "0.1.3", features = [ "time" ] }
  tokei           = "13.0.0-alpha.8"
  typetag         = "0.2"

[dev-dependencies]
  criterion     = "0.5.1"
  gix-testtools = "0.15.0"
  insta         = { version = "1.42.1", features = [ "json", "redactions" ] }
  rstest        = "0.24.0"

[[bench]]
  harness = false
  name    = "repo"

[build-dependencies]
  lazy_static = "1"
  regex       = "1"
  serde_json  = "1"
  serde_yaml  = "0.9"
  tera        = { version = "1", default-features = false }

[target.'cfg(windows)'.build-dependencies]
  winres = "0.1"

[target.'cfg(windows)'.dependencies]
  enable-ansi-support = "0.2.1"

[features]
  fail-on-deprecated = [  ]

@spenserblack
Copy link
Collaborator

@Sk7Str1p3 Try to separate formatting from behavioral changes (features, fixes) in your commits. I.e. formatting should in a different commit from other tasks. The reason is that formatting commits can be added to .git-blame-ignore-revs. I'm happy to explain this further if needed 🙂

@Sk7Str1p3
Copy link
Contributor Author

Oh yes, I accidentally pushed formatted files, sorry.

Copy link

@cardboardcpu cardboardcpu left a comment

Choose a reason for hiding this comment

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

Maybe it could be interesting to consider treefmt-nix since it goes well with flakes.

P.D: Gotta love being able to comfortably build this project from latest git :p

@Sk7Str1p3
Copy link
Contributor Author

Maybe it could be interesting to consider treefmt-nix since it goes well with flakes.

It goes with flakes really well, I'm really tired of manually formatting toml, bringing taplo to shell and format every single file manually.

Also I think we should bring here flake-parts

@Sk7Str1p3
Copy link
Contributor Author

@spenserblack will it be okay to have pull requests with same name like 'flake.lock: update'? I (almost) succeeded with workflow but I couldn't came up with idea how should we distinguish them. Do you have any ideas on that?

@Sk7Str1p3
Copy link
Contributor Author

OH I MANAGED TO ADD DATE THERE YAY

@Sk7Str1p3
Copy link
Contributor Author

@o2sh @spenserblack, PR is ready for merge, but...

Main part is finished, but we can use all of this nix features in our CI. main advantage is that we can reuse built deps and let do it for everyone (who uses nix obviously), so this will leed to much faster, really blazing fast builds and checks. I can try implement all of these.

So, what's your opinion on that?

@spenserblack
Copy link
Collaborator

I don't think we should use Nix in the CI. CI should, IMO, be close to a standard user building the project. It's like proving "can this project be built" vs "can this project be built with Nix".

However, a new CI job to validate that the Nix configuration is good might make sense. But I think that would be for a default.nix, wouldn't it?

Copy link
Collaborator

@spenserblack spenserblack left a comment

Choose a reason for hiding this comment

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

Mostly looks good to me, left a few recommended changes for grammatical issues. Though TBH I'm hesitant on merging because of:

  • Changes to cargo audit configuration
  • (IMO) unrelated formatting changes that diff a lot of the Cargo.toml

I'll pass this along to @o2sh. If they think those issues aren't a problem, I think we can move forward.

Comment on lines +36 to +37
# This filter prevent project from being rebuilded then changing
# unrelated files ,e.g. README
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# This filter prevent project from being rebuilded then changing
# unrelated files ,e.g. README
# This filter prevents the project from being rebuilt when changing
# unrelated files, e.g. README

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I'm bad with English, sorry. I'll fix soon

inherit src;
strictDeps = true;

# Bunch of libraries required for package proper work
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# Bunch of libraries required for package proper work
# Bunch of libraries required for package to properly work

@spenserblack spenserblack requested a review from Copilot July 29, 2025 12:28
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request introduces a comprehensive Nix-based development and build pipeline for the Onefetch project. It enables development environments, automated builds, code quality checks, and CI integration through Nix flakes.

  • Adds Nix flake configuration with development shell, build derivations, and automated checks
  • Implements code formatting standardization across Rust and TOML files
  • Sets up automated dependency updates via GitHub Actions workflow

Reviewed Changes

Copilot reviewed 12 out of 15 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
flake.nix Main Nix flake defining build configuration, checks, and development environment
treefmt.nix Tree formatter configuration for Rust and Nix code formatting
taplo.toml TOML formatter configuration with alignment and reordering settings
deny.toml Cargo deny configuration restricting licenses to MIT only
.envrc Direnv configuration to automatically load Nix development shell
.cargo/audit.toml Cargo audit configuration disabling yanked crate checks for sandbox compatibility
.github/workflows/flake.yml GitHub Actions workflow for automated weekly flake.lock updates
Various Cargo.toml files Reformatted according to new TOML formatting rules

Comment on lines +36 to +37
# This filter prevent project from being rebuilded then changing
# unrelated files ,e.g. README
Copy link
Preview

Copilot AI Jul 29, 2025

Choose a reason for hiding this comment

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

Grammar error: 'prevent project from being rebuilded then changing' should be 'prevents the project from being rebuilt when changing'

Suggested change
# This filter prevent project from being rebuilded then changing
# unrelated files ,e.g. README
# This filter prevents the project from being rebuilt when changing
# unrelated files, e.g., README

Copilot uses AI. Check for mistakes.

craneLib = crane.mkLib pkgs;

# This filter prevent project from being rebuilded then changing
# unrelated files ,e.g. README
Copy link
Preview

Copilot AI Jul 29, 2025

Choose a reason for hiding this comment

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

Formatting issue: there should be no space before the comma in ',e.g.'

Suggested change
# unrelated files ,e.g. README
# unrelated files, e.g. README

Copilot uses AI. Check for mistakes.

tomlFmt = craneLib.taploFmt {
src = pkgs.lib.sources.sourceFilesBySuffices src [ ".toml" ];
# taplo arguments can be further customized below as needed
# taploExtraArgs = "--config ./taplo.toml";
Copy link
Preview

Copilot AI Jul 29, 2025

Choose a reason for hiding this comment

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

The commented taplo configuration line references './taplo.toml' but the actual taplo.toml file exists at the project root. This should be uncommented and the path corrected to just 'taplo.toml' or removed if not needed.

Suggested change
# taploExtraArgs = "--config ./taplo.toml";
taploExtraArgs = "--config taplo.toml";

Copilot uses AI. Check for mistakes.

inherit cargoArtifacts;
partitions = 1;
partitionType = "count";
cargoNextestPartitionsExtraArgs = "--no-tests=pass";
Copy link
Preview

Copilot AI Jul 29, 2025

Choose a reason for hiding this comment

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

The cargo-nextest argument '--no-tests=pass' appears incorrect. The correct nextest argument should be '--fail-fast' or similar. '--no-tests' is not a valid nextest option.

Suggested change
cargoNextestPartitionsExtraArgs = "--no-tests=pass";
cargoNextestPartitionsExtraArgs = "--fail-fast";

Copilot uses AI. Check for mistakes.

default = onefetch-debug;
};

apps.default = flake-utils.lib.mkApp { drv = (build // { CARGO_PROFILE = "release"; }); };
Copy link
Preview

Copilot AI Jul 29, 2025

Choose a reason for hiding this comment

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

The attribute set merge syntax '(build // { CARGO_PROFILE = "release"; })' is incorrect here. The 'build' derivation cannot be merged with environment variables this way. This should reference the 'onefetch' package from packages instead.

Suggested change
apps.default = flake-utils.lib.mkApp { drv = (build // { CARGO_PROFILE = "release"; }); };
apps.default = flake-utils.lib.mkApp { drv = self.packages.${system}.onefetch; };

Copilot uses AI. Check for mistakes.

@@ -0,0 +1,2 @@
[licenses]
allow = [ "MIT" ]
Copy link
Preview

Copilot AI Jul 29, 2025

Choose a reason for hiding this comment

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

[nitpick] Restricting licenses to only MIT may be overly restrictive and could prevent using dependencies with compatible licenses like Apache-2.0, BSD-3-Clause, or ISC. Consider allowing additional permissive licenses.

Suggested change
allow = [ "MIT" ]
allow = [ "MIT", "Apache-2.0", "BSD-3-Clause", "ISC" ]

Copilot uses AI. Check for mistakes.

@Sk7Str1p3
Copy link
Contributor Author

Completely forgot about all this lol. I'll fix typos in few minutes

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.

4 participants