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

Generalize nixConfig attributes to systems #5993

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

oldaccountdeadname
Copy link
Contributor

See #5989 for context. The implementation here should be pretty straightforward.

Assuming no outstanding implementation issues, how can (should?) I write tests for these changes? Given the inherent host environment impurity, I'm not sure where to start with it.

@edolstra
Copy link
Member

See my comment here: #5989 (comment)

Regarding the implementation: applyAttrs() shouldn't depend on the system type. Rather, ConfigFile should be extended with a std::map<std::string, std::map<std::string, ConfigValue>> settingsPerSystem field.

The name applyAttrs() is confusing given the apply() method, since it doesn't apply any settings. Probably parseAttrs() is better.

@oldaccountdeadname
Copy link
Contributor Author

@edolstra - I modified and force-pushed the patchest given your feedback; hopefully it's a bit cleaner now. The git history is similar to the previous iteration, but the perSystem attribute is implemented in a commit following the one allowing system specific changes in the first place.

This refactor is a natural extension of encapsulation, and provides more
visual room for both the caller (getFlake) and parseAttrs itself.

Functionality is unchanged.
System-specific config settings are read into ConfigFile::settingsBySys,
and only applied by ConfigFile::apply if the system is either none (used
to represent system-agnostic settings) or the current system.
Previous commits allowed the following, but this is undesired per the
discussion in NixOS#5989.

	nixConfig.x86_64-linux.<opt> = <val>;

This commit ammends the implementation to exclusively accept options
like the following:

	nixConfig.perSystem.x86_64-linux.<opt> = <val>;
See discussion in 5989. We want to have system-agnostic settings
override global /etc/nix settings, but we want to combine
system-specific settings with non-global system-agnostic settings.
@oldaccountdeadname
Copy link
Contributor Author

Rebased on current master and added documentation.

@stale stale bot added the stale label Jul 31, 2022
@stale stale bot removed the stale label Jun 23, 2023
@Ericson2314
Copy link
Member

CC @roberth I suspect this should be linked to the other issues trying to solve flake system problems?

@roberth
Copy link
Member

roberth commented Jun 23, 2023

What is it for?

The original use case (aarch64-darwin.extra-platforms = [ "x86_64-darwin" ];) seems to have been fixed for many Nix releases now. Judging by the linked issue, I'm skeptical of the complexity this adds.

@Ericson2314 idk where to link for this specifically. It seems that this shares the problem of having to enumerate systems, and this only adds to the problem.

Regarding the implementation: applyAttrs() shouldn't depend on the system type.

Maybe it used to be a function? Sad if true.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants