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

Add pure-storepath-builtin experimental feature #12141

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

Conversation

Pandapip1
Copy link
Contributor

Motivation

No consensus has been found as to whether storePath should be considered pure or impure. As such, an experimental feature should be added to allow those that consider the function pure to use it as such.

Context

See #5868.

Heads up: This is my first code change to nix; it's likely I missed something from the contributing document (even though I did read through it), and I couldn't find any documentation about the process of adding an experimental feature in the repo (I was on an airplane with no internet and am just now submitting the PR).


Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

@Pandapip1 Pandapip1 force-pushed the pure-builtins-storePath branch 3 times, most recently from af76797 to 56b63e2 Compare January 6, 2025 18:34
@Ericson2314
Copy link
Member

Heh, I would support this. It is sort of "unsure whether to stabilize as de facto stability 2nd time as farce", but shrug.

@Pandapip1
Copy link
Contributor Author

Should I just link to #5868 as the tracking issue temporarily so that CI is happy? I don't think a 'real' tracking issue is really warranted since it'd be mostly discussions about whether to make storePath pure or not, and we already have that as #5868.

@Pandapip1 Pandapip1 force-pushed the pure-builtins-storePath branch from 56b63e2 to 7d6fdb9 Compare January 12, 2025 19:30
@Pandapip1 Pandapip1 marked this pull request as ready for review January 13, 2025 00:17
@roberth
Copy link
Member

roberth commented Jan 14, 2025

It feels more like a preference to me, which feels comparable to nixpkgs.allowUnfree in terms of the effect it has, and with a potential slight overlap in the kind of thing it prevents.

This seems to be consistent with the lack of a milestone. I think an experiment should be set up to have some sort of outcome, but it seems that in this case it comes down to personal preferences and or the requirements of individual projects.

If we were to make this an individual setting instead, I believe this would have the following benefits over an experimental feature flag:

  • no buildup of feature flags that won't be removed
  • no migration required when we eventually turn this into a setting anyway
  • ability to configure this in flake.nix/nixConfig
  • shorter CLI flag

Perhaps worth noting about the disabled setting

  • it doesn't make much sense for global configuration, because addToStore is always allowed
  • doesn't prevent appendContext, which has a similar effect to storePath, but does not allow use of the path's content in evaluation when in pure mode (i.e. IFD not allowed, but can install a pre-evaluated package or binary store path)

@Ericson2314 @Pandapip1 wdyt?

@Pandapip1
Copy link
Contributor Author

Pandapip1 commented Jan 14, 2025

no buildup of feature flags that won't be removed

The plan would be that this would be removed.

no migration required when we eventually turn this into a setting anyway

I really hope this does not become a permanent setting because otherwise this is something that people have to build around, and that seems bad. This is pretty explicitly something that will only exist while there remains debate about the storePath built-in. Eventually, either it will become pure or impure.

ability to configure this in flake.nix/nixConfig

I'm not entirely convinced this is an upside, but I'm also not entirely convinced it's a downside either.

shorter CLI flag

While this is true, I can't imagine that people would not just have this configured as a global setting.


I do not have particularly strong opinions about whether or not it should be a setting or an experimental feature, and I'll just go with whichever result seems to reflect the popular opinion to get this merged faster.

@roberth
Copy link
Member

roberth commented Jan 14, 2025

If we're ok to accept the feature when no significant issues are found during the experiment, then indeed that could be a better outcome than a setting.
Also a setting would still be a possibility after the experiment, so this choice is not critical at all.
Unless someone has a convincing argument, I'd be ok to make it experimental, to avoid getting stuck again.

@edolstra
Copy link
Member

I feel an experiment wouldn't reveal anything that we don't already know. The issue with builtins.storePath isn't implementation/interface, it's a design question of whether we want to allow this kind of compromise to purity.

IMHO a setting (allow-impure-store-paths, defaulting to false) would be better.

@roberth
Copy link
Member

roberth commented Jan 15, 2025

Content addressed store paths are neither

I don't think we should further dilute the term.

How about something like allowed-store-path-dependencies, with possible values

  • none
  • content-addressed to allow CA paths (sources or floating outputs)
  • built to allow the above and input addressed derivations
  • impure to allow the above + impure derivations

Still not sure about -dependencies, have considered -literals. Maybe -values?

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

Successfully merging this pull request may close these issues.

4 participants