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

Revert auto-channel detection that produced irrecoverable edge-cases #89

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

Conversation

blaggacao
Copy link
Collaborator

In more advanced usage scenarios, the guarantees of this code path
are never strong enough and the error produced is non-recoverable.

Sorry to lovers of "automagic".

@blaggacao blaggacao force-pushed the fix-too-much-magic-with-inputs branch 3 times, most recently from 5993edf to 6f261d7 Compare September 5, 2021 08:26
@blaggacao blaggacao mentioned this pull request Sep 5, 2021
@Pacman99
Copy link
Collaborator

Pacman99 commented Sep 5, 2021

Can you provide an example of such an irrecoverable error, to strengthen the argument for this change.

gytis-ivaskevicius and others added 3 commits September 5, 2021 19:18
In more advanced usage scenarios, the guarantees of this code path
are never strong enough and the error produced is non-recoverable.

Sorry to lovers of "automagic".
@blaggacao blaggacao force-pushed the fix-too-much-magic-with-inputs branch from 6f261d7 to a4e267e Compare September 5, 2021 16:36
Base automatically changed from staging to master September 5, 2021 16:41
@blaggacao
Copy link
Collaborator Author

blaggacao commented Sep 5, 2021

Can you provide an example of such an irrecoverable error, to strengthen the argument for this change.

filterAttrs (_: value: value ? legacyPackages && value.legacyPackages.x86_64-linux ? nix) inputs;

any item that unintentionally falls throgh that predicate will suffice.

With the current implementation, any repo that bears legacyPackages.x86_64-linux.nix and is not nixpkgs is such a repo.

But pushing this formally incorrect code further to the edges, would do little more than just modifying the predicate for that failure scenario.

I'm in a situation a.t.m. where I can't accept looming regressions for my own "safety/sanity". Other people might not be in that situation and hence be a little more forgiving.

I think my best argument — other than "it actually breaks" — is that if we want to promote more serious and higher stake adoption of fup we should aim at a formally correct, easily auditable, feature limited version of fup.

EDIT: oh and the irrecoverable breakage of course happens when fup tries to import such repo with nixpkgs' import signature.

It's just a mismatch between asked and offered guarantees within the same code-base.

@blaggacao
Copy link
Collaborator Author

blaggacao commented Sep 5, 2021

Here is another good one:

The expressivity to be able to canonically encode the intent to not use a nixpkgs type input also as a channel must be preserved for a low level lib.

Use case: don't encourage your users to use a channel you never intended to expose.

Copy link
Collaborator

@Pacman99 Pacman99 left a comment

Choose a reason for hiding this comment

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

I'm fine with this, I see the edge case problems. I figure that it would just be better to not infer and just do inputs.${channelName} so that would just trigger a nix error saying that the attribute doesn't exist in inputs immediately guiding the user to the problem. I think the current inference methods are cool but extra.

I also personally don't think this kind of magic should be here anyways, but I'm alright with it if done right.

@mstrangfeld
Copy link

Just wanted to add that this currently happens when having the current version of agenix in the inputs. Was pretty hard to figure out what was actually causing it... 😅

error: attribute 'x86_64-linux' missing

       at /nix/store/i5ipw8rvj1fvdaj01bqzc0dpw9irrb4h-source/lib/mkFlake.nix:208:74:

          207|         # For some odd reason `devshell` contains `legacyPackages` out put as well
          208|         channelFlakes = filterAttrs (_: value: value ? legacyPackages && value.legacyPackages.x86_64-linux ? nix) inputs;
             |                                                                          ^
          209|         channelsFromFlakes = mapAttrs (name: input: { inherit input; }) channelFlakes;

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.

4 participants