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

om health: Add supported nix versions health check #398

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

shivaraj-bh
Copy link
Member

@shivaraj-bh shivaraj-bh commented Jan 22, 2025

resolves #386

Changes

  • Introduces version specification system for NixVersion
  • Replaces nix-versions.min-required health check with more flexible nix-versions.supported

The new version specification system provides APIs to parse complex version requirements, enabling fine-grained control over which Nix versions should pass the health check.

Example of a version requirement string for nix-versions.supported:

“>=2.16.0, !=2.23.10, <2.25"

This requirement specifies that:

  • Versions must be at least 2.16.0
  • Must not be exactly 2.23.10
  • Must be less than 2.25

Any Nix version meeting all these criteria will pass the health check.

TODO

@shivaraj-bh shivaraj-bh marked this pull request as draft January 22, 2025 19:37
@shivaraj-bh
Copy link
Member Author

Check passing with:

health:
  default:
    nix-version:
      supported:
        - ">=2.16.0"
...
✅ Flakes Enabled
experimental-features = flakes fetch-tree nix-command
✅ Minimum Nix Version
nix version = 2.24.10
✅ Supported Nix Versions
nix version = 2.24.10
...
✅ All checks passed

Check failing with:

health:
  default:
    nix-version:
      supported:
        - ">=2.16.0"
        - "<2.24.10, >2.24.10"
...
✅ Flakes Enabled
experimental-features = flakes fetch-tree nix-command
✅ Minimum Nix Version
nix version = 2.24.10
❌ Supported Nix Versions
nix version = 2.24.10
Problem: Your Nix version (2.24.10) doesn't satisfy the supported bounds: >=2.16.0, <2.24.10, >2.24.10
Fix:     Set nix.package in home-manager to the desired Nix version
...
❌ Some required checks failed

@srid
Copy link
Member

srid commented Jan 22, 2025

  • Do we still have to support nix-version.min-required? Even if we are to deprecate it, how to do it?

Depends on the behaviour when we run this against old code (which specifies min-required). What is the output in that case?

@shivaraj-bh
Copy link
Member Author

Depends on the behaviour when we run this against old code (which specifies min-required). What is the output in that case?

It is silently ignored.

@shivaraj-bh
Copy link
Member Author

An example of failure using the new NixVersion spec:

image

#[derive(Debug, Serialize, Deserialize, PartialEq, Clone)]
#[serde(default, rename_all = "kebab-case")]
pub struct NixVersionCheck {
pub supported: String,
Copy link
Member Author

Choose a reason for hiding this comment

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

I wish serde supported passing a function as a deserializer. I could just use pub supported: NixVersionReq here, if it did. Maybe it does and I haven’t looked properly?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wish serde supported passing a function as a deserializer. I could just use pub supported: NixVersionReq here, if it did. Maybe it does and I haven’t looked properly?

I can also avoid using unwrap() in the check function below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe #[serde(deserialize_with = “<function-name>”)]?

Copy link
Member Author

Choose a reason for hiding this comment

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

53355ba

works

/// Patch version
pub patch: u32,
pub patch: Option<u32>,
Copy link
Member

Choose a reason for hiding this comment

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

This changes the semantics of this type. If you look at the type documenation, it says

Nix version as parsed from `nix --version`

And nix --version always returns all 3 components - major, minor and patch.


It seems that the type here is being changed to fit something in version_spec.hs - yet version.rs is isolated from that module.

@@ -0,0 +1,204 @@
//! Rust module defining the spec for [NixVersion]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
//! Rust module defining the spec for [NixVersion]
//! Version requirement spec for [NixVersion]

Neq(NixVersion),
}

/// A collection of version requirements
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// A collection of version requirements
/// Version requirement for [NixVersion]
///
/// Example:
/// ```
/// >=2.8, <2.14, 12.13.4
/// ```


use crate::version::{BadNixVersion, NixVersion};

/// Specifies a version requirement for Nix
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Specifies a version requirement for Nix
/// An individual component of `NixVersionReq`

/// Checks if all version specifications are satisfiable
///
/// Returns Err if the requirements are logically impossible to satisfy
fn check(specs: &[NixVersionSpec]) -> Result<(), BadNixVersionSpec> {
Copy link
Member

Choose a reason for hiding this comment

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

I was confused by this function for a moment.

Suggested change
fn check(specs: &[NixVersionSpec]) -> Result<(), BadNixVersionSpec> {
fn ensure_logically_sound(specs: &[NixVersionSpec]) -> Result<(), BadNixVersionSpec> {

Copy link
Member

Choose a reason for hiding this comment

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

By the way, why do we need this check? Asked differently: what happens if we don't use it?

NixVersionSpec::Gt(v) => version > v,
NixVersionSpec::Gteq(v) => version >= v,
NixVersionSpec::Lt(v) => version < v,
NixVersionSpec::Lteq(v) => version < v,
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be <=?

Comment on lines +131 to +141
use NixVersionSpec::{Gt, Gteq, Lt, Lteq, Neq};

let s = s.trim();
match s.get(..2) {
Some("!=") => Ok(Neq(NixVersion::from_str(&s[2..])?)),
Some(">=") => Ok(Gteq(NixVersion::from_str(&s[2..])?)),
Some("<=") => Ok(Lteq(NixVersion::from_str(&s[2..])?)),
Some(s2) if s2.starts_with('>') => Ok(Gt(NixVersion::from_str(&s[1..])?)),
Some(s2) if s2.starts_with('<') => Ok(Lt(NixVersion::from_str(&s[1..])?)),
_ => Err(BadNixVersionSpec::UnknownOperator(s.to_string())),
}
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, yea, parsing in Rust can be more complicated than Haskell.

I don't understand (in immediate glance; without thinking much) what this 2 index is about. Would using a parsec based parser simplify things? e.g.: https://github.com/Marwes/combine

Comment on lines +159 to +166
let mut specs = self.specs.iter();

if let Some(first) = specs.next() {
write!(f, "{}", first)?;
for spec in specs {
write!(f, ", {}", spec)?;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

join instead of iter?

pub supported: NixVersionReq,
}

fn deserialize_version_req<'de, D>(deserializer: D) -> Result<NixVersionReq, D::Error>
Copy link
Member

Choose a reason for hiding this comment

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

let is_supported = self.supported.specs.iter().all(|spec| spec.matches(val));

let supported_version_check = Check {
title: "Supported Nix Versions".to_string(),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
title: "Supported Nix Versions".to_string(),
title: "Nix version is supported".to_string(),

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.

om health(nix-version): Add vulnerableVersions
2 participants