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

Use YAML for config #352

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

Use YAML for config #352

wants to merge 7 commits into from

Conversation

shivaraj-bh
Copy link
Member

resolves #343

If the flake url is a local path and om.yaml exists in the project root, omnix will not invoke nix eval .#om.

om.yaml Outdated Show resolved Hide resolved
Copy link
Member

@srid srid left a comment

Choose a reason for hiding this comment

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

This looks good so far. Can we improve the OmConfig::get interface so as to resolve and avoid unnecessary nix command runs? See #339 (comment)

/// Fetch the `om` configuration from `om.yaml` if present, falling back to `om` config in flake output
pub async fn get(cmd: &NixCmd, flake_url: &FlakeUrl) -> Result<Self, OmConfigError> {
match Self::from_yaml(cmd, flake_url).await {
Err(OmConfigError::YamlNotFound(_)) => Self::from_flake(cmd, flake_url).await,
Copy link
Member

@srid srid Nov 26, 2024

Choose a reason for hiding this comment

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

I just realized I got confused (see "catchall" comment here) by YamlNotFound. Because it a value of OmConfigError, I somehow confused it to refer to yaml errors.

Just get rid of YamlNotFound and have from_yaml return Option<Self> instead, so the caller can do fallback on None.

Copy link
Member Author

Choose a reason for hiding this comment

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

If from_yaml returns Option<Self> will we not be silently skipping over errors while parsing the YAML?

Copy link
Member

Choose a reason for hiding this comment

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

No, I meant:

    async fn from_yaml(cmd: &NixCmd, flake_url: &FlakeUrl) -> Result<Option<Self>, OmConfigError> {

@shivaraj-bh
Copy link
Member Author

This looks good so far. Can we improve the OmConfig::get interface so as to resolve and avoid unnecessary nix command runs? See #339 (comment)

Aren’t those nix commands the result of the checks in preShell?

pub async fn develop_on_pre_shell(prj: &Project) -> anyhow::Result<()> {
// Run relevant `om health` checks
let health = NixHealth::from_om_config(&prj.om_config)?;
let nix_info = NixInfo::get()
.await
.as_ref()
.with_context(|| "Unable to gather nix info")?;
let mut relevant_checks: Vec<&'_ dyn Checkable> =
vec![&health.nix_version, &health.rosetta, &health.max_jobs];
if !health.caches.required.is_empty() {
relevant_checks.push(&health.trusted_users);
};

@srid
Copy link
Member

srid commented Nov 27, 2024

Aren’t those nix commands the result of the checks in preShell?

While we can't avoid running nix commands in preShell (for now), we certainly can avoid that in postShell (which too will be run as part of direnv invocation):

image

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.

YAML for config
2 participants