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

Improve "illegal path references in fixed output derivation" error #12

Closed

Conversation

bmillwood
Copy link

This was just merged into nix, see:

I've asked about this on Matrix and to the best of my knowledge there's no automatic means by which Nix patches make it over here, so I thought I'd just make the PR again. Happy for this to be closed / ignored if the change would make it into Lix eventually regardless.

I also haven't specifically tested this in Lix. I tested that it built but since it's a simple change, I'm hoping you'll find the Nix testing sufficient. Let me know if not (though realistically I might not have time to do more particularly soon.)

Motivation

Improving a somewhat-maligned Nix error message (the issue above has links to people complaining about this one).

The main improvement is that the new message gives an example of a path
that is referenced, which should make it easier to track down. While
there, I also clarified the wording, saying exactly why the paths in
question were illegal.
@bmillwood bmillwood marked this pull request as ready for review January 27, 2025 00:46
@sofiedotcafe
Copy link

+1

@lf-
Copy link
Contributor

lf- commented Mar 2, 2025

Oh dear we didn't notice this because this is not the recommended way to contribute to lix (gerrit.lix.systems is the normal way) and it didn't get mentioned in the issue suggesting we should fix this either. I'll try to remember to deal with it at work tomorrow.

Thank you for porting this!

Copy link
Contributor

@lf- lf- left a comment

Choose a reason for hiding this comment

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

This is the bug report on our end: https://git.lix.systems/lix-project/lix/issues/530

I don't think this is necessarily a good solution: it shows just one path for some reason (saving screen space?), and the paths that are checked for are a quite finite list: it's the inputs of the FOD. So I don't know why one would limit it to just one.

@bmillwood
Copy link
Author

Oh dear we didn't notice this because this is not the recommended way to contribute to lix (gerrit.lix.systems is the normal way) and it didn't get mentioned in the issue suggesting we should fix this either. I'll try to remember to deal with it at work tomorrow.

No worries :) I'm not at all in a rush. I know this isn't the ideal way to contribute but I don't think I'll become a serial contributor (to either Lix or Nix) so I didn't fancy creating a new user account just for this little patch.

it shows just one path for some reason (saving screen space?)

The reason is that I initially intended it to just be a kind of starting point for a discussion of a better solution, so I didn't try very hard; I am neither a Nix/Lix native nor a C++ native, so I didn't know how to do something better and didn't fancy spending the time to learn.

I think mostly the code in the issue you link is better, although I like my wording change more, so maybe combining both would be best? But I have even less time to contribute to this than I did when I made the patch, so mostly this is up to you now I think :)

@Tom-Hubrecht
Copy link
Contributor

I opened https://gerrit.lix.systems/c/lix/+/2726 and added you as co-author

@bmillwood
Copy link
Author

bmillwood commented Mar 4, 2025

I opened https://gerrit.lix.systems/c/lix/+/2726 and added you as co-author

nice! it's now different enough from my patch that I don't know if I deserve the credit, but I guess it doesn't cost anything :)

@RaitoBezarius
Copy link
Contributor

Closing in favor of the Gerrit CL, let's continue the discourse there. Many thanks to everyfew here! You all are awesome.

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.

5 participants