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

Flakes: Add "" as the first prefix to try #5657

Closed
wants to merge 1 commit into from
Closed

Conversation

L-as
Copy link
Member

@L-as L-as commented Nov 25, 2021

Fixes #5651

@edolstra
Copy link
Member

Isn't it better to just try the prefixes returned by getDefaultFlakeAttrPathPrefixes() after the unprefixed attr? Now it's presumably going to try the unprefixed attr twice.

@L-as
Copy link
Member Author

L-as commented Nov 25, 2021

You're right. I couldn't make much sense of where the prefixes are defined and used, and this seemed like the easiest solution, though it is inadequate. I still think the best solution is to have the "" be the first entry in the list of prefixes to try, such that users of this list shouldn't have to try that case explicitly (assuming I'm not misunderstanding the code).

@tomberek
Copy link
Contributor

A complication comes from different commands looking in different places for their preferred attrs. eg: “nix develop” looks in devShell, but should “nix develop .#myshell” first check outputs.devShell.myshell or outputs.myshell?

The defaults in getDefaultFlakeAttrPathPrefixes are most applicable to things like “nix build”.

@L-as
Copy link
Member Author

L-as commented Nov 26, 2021

outputs.myshell IMO. If you choose the other approach, then there's not a way currently of disambiguating it.

@lilyball
Copy link
Member

This is going to make nixpkgs#lib refer to the top-level lib in the flake instead of the lib in the package set. At the very least this means any overlays that update lib wouldn't apply to nixpkgs#lib after this change; I don't know if there are any other practical differences in this case besides the fact that the top-level lib has a nixosSystem function. But it's an example of a potentially surprising outcome of this change.

More generally, if nixpkgs#hello resolves to nixpkgs#legacyPackages.aarch64-darwin.hello then nixpkgs#foo should resolve to nixpkgs#legacyPackages.aarch64-darwin.foo. I shouldn't have to know the complete set of outputs of a flake to predict where arbitrary fragments resolve to. Also as a reproducibility issue, a flake adding a new top-level output shouldn't break existing flake references (e.g. nixpkgs adding a top-level hello output shouldn't break nixpkgs#hello).

I feel like perhaps a better approach is to have a separate syntax for "don't apply any prefixes to this fragment". For example, something like nixpkgs##lib.

@L-as
Copy link
Member Author

L-as commented Nov 27, 2021

I don't think "reproducibility" is an issue here, because this is for the CLI. The CLI commands necessary to do something might change over time, and isn't important at all IMO. Reproducibility happens through the flake itself.

Adding a whole new syntax for this is quite overkill IMO, and x#y already tries various different paths within x. It is IMO the responsibility of the author of the flake to make sure that no confusion happens. If they choose to add a top-level attribute that shadows an existing one, that's their choice, and is hardly something that happens by accident.

I'm having a hard time imagining any scenario where this behavior would be undesirable.

@lilyball
Copy link
Member

It's for the CLI, but the CLI is the interface used by scripting. Besides, predictability is important, not just for making things work, but also for understanding what happened when something went wrong.

I'm having a hard time imagining any scenario where this behavior would be undesirable.

It prevents the flake author from adding outputs if they would conflict with fragments that are currently valid. It also prevents flakes from adding packages whose names match those of existing outputs without those packages being hard to reference. For example, if someone adds a package checks to nixpkgs, then nixpkgs#checks could not be used to reference it as the nixpkgs flake already has an output named checks.

More generally, the flake#fragment syntax for the CLI seems mostly intended to be a DWIM "access something from the correct output" syntax. If a flake has e.g. a packages.<system> output, then flake#fragment is expected to access a package (or an app if there's apps.<system>). The fact that it falls back to unprefixed outputs is behavior that most users aren't going to be relying on.

I do think there's a need for being able to reliably refer to outputs by name. But saying "prioritize these over the prefixed outputs" is ignoring the fact that most users aren't trying to access outputs by name, and it will just end up being a footgun. So either we should have a separate syntax that means "don't prefix", or we should have some separate means of doing this, such as a --flake name flag similar to --expr expr that means "interpret installables as attribute paths relative to the flake name" that skips the prefix stuff. Personally, I think the "modified syntax" form is simpler.

@L-as
Copy link
Member Author

L-as commented Nov 27, 2021

I think this issue is made worse by the fact that builtins.getFlake "." doesn't work, so you can't disambiguate easily using --expr.

@stale
Copy link

stale bot commented Jun 12, 2022

I marked this as stale due to inactivity. → More info

@stale stale bot added the stale label Jun 12, 2022
@balsoft
Copy link
Member

balsoft commented Jan 20, 2023

CC @lilyball

This is going to make nixpkgs#lib refer to the top-level lib in the flake instead of the lib in the package set. At the very least this means any overlays that update lib wouldn't apply to nixpkgs#lib after this change

When using nixpkgs flake on the CLI, no overlays are applied anyways (unless you have nixpkgs-overlays and are doing --impure, which is a bad idea)

More generally, if nixpkgs#hello resolves to nixpkgs#legacyPackages.aarch64-darwin.hello then nixpkgs#foo should resolve to nixpkgs#legacyPackages.aarch64-darwin.foo

It currently does not do this necessarily. It can also resolve to nixpkgs#packages.aarch64-darwin.foo for example. Or, if foo is missing from both legacyPackages and packages, it can resolve to nixpkgs#foo.

I shouldn't have to know the complete set of outputs of a flake to predict where arbitrary fragments resolve to.

You already have to, see above.

The fact that it falls back to unprefixed outputs is behavior that most users aren't going to be relying on.

Why? I think it's really important to have the ability to select any flake output from the flake, which is impossible in general with the way things currently are. If someone, as in your example, adds a checks package to nixpkgs, then suddenly you can't access the checks output, e.g. with nix eval or other tools which can happily work with any attrset.

The fact that flake authors should mind their outputs is, in my mind, much less important then flake users being completely unable to access certain outputs sometimes.

@stale stale bot removed the stale label Jan 20, 2023
@balsoft
Copy link
Member

balsoft commented Jan 20, 2023

(also, I think this behavior should be disabled for more low-level tools such as nix eval and nix show-derivation)

@tomberek
Copy link
Contributor

We’ve been playing with this concept as a way to signal to the CLI that there should be no automatic resolution or searching:

flakeref#.attrPath

it is also helpful for performance and usage in scripts. Sometimes you don’t want to eval legacyPackages searching for an attrPath when you already know the fully resolved path.

Ref: master...flox:nix:disable_attrpath_resolution

@Ericson2314
Copy link
Member

Ericson2314 commented Aug 4, 2023

Triaged in the Nix Team meeting this morning (n.b. I could not make it):

Alternative: flag or syntax to make the flake paths absolute (both existing as PRs)

  • @edolstra: Could even have a flag to fully specify the prefixes to search into. Cloud be useful for scripts (nixos-rebuild in particular)

For this PR:

  • Need to make sure it's not looking at the root twce
  • Otherwise, good

Assigned to @roberth

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-08-04-nix-team-meeting-minutes-77/31487/1

@roberth
Copy link
Member

roberth commented Oct 6, 2023

@L-as iirc when we discussed this, you thought absolute paths are a good alternative. I think the arguments were:

  • The current behavior is consistent with the semantics of PATH.
  • Explicit is good. If you want absolute path behavior, you can now prefix a ..

Did I overlook something, or shall we close this?

@L-as
Copy link
Member Author

L-as commented Oct 6, 2023

Backward-compatibility is important, even for the CLI.

@L-as L-as closed this Oct 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Flake fragments should prioritise the top-most attribute.
9 participants