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 moduleLocation to mkFlake argument #158

Merged
merged 5 commits into from
Oct 29, 2023
Merged

Conversation

Atry
Copy link
Contributor

@Atry Atry commented May 9, 2023

This PR adds moduleLocation argument to mkFlake, allowing the user to set the module key by hand, addressing #156 (comment)

@Atry
Copy link
Contributor Author

Atry commented May 9, 2023

@roberth Would you mind if I reopen #156 and rebase onto this PR, so that we don't have to break inputs.self in order to call mkFlake twice.

@roberth
Copy link
Member

roberth commented May 9, 2023

I meant to say "in the module files" or something; I've edited #156 (comment) to reflect this. It was supposed to be a solution for the problem of having no keys when doing local imports.

I don't think the manipulation of self is worth solving, because double mkFlake is not quite sufficient for the purpose of dogfooding.

@roberth
Copy link
Member

roberth commented May 9, 2023

#157 is worth solving, but unless we know that we have a complete solution, we're probably just making a mess out of mkFlake.

We already have a "95%" solution of the problem and I am confident that a full solution of #157 is within reach. For this reason, I'm not going to accept a solution that solves a different 95%, and for the sake of maintainability I won't accept a 96% solution either, because that solution will not come with a proof that it leads towards the 100%. If it does come with such a proof, then it's already a 100% solution.

I've already stretched my schedule a lot by responding to your suggestions. I won't be responding quickly for some time, but that won't be too bad, because you can test changes yourself (see e.g. the dev directory) and you can validate the requirements or better understand them by applying your solution in multiple repositories. I'm sorry that I won't be quite as helpful for now, but I have to prioritize.

@Atry
Copy link
Contributor Author

Atry commented May 9, 2023

This PR does not aim to directly solve #157. Instead, it is a workaround for #148. I understand the best solution of #148 is NixOS/nix#4154. But we probably would not get NixOS/nix#4154 very soon because it is inactive.

I think it would be a net benefit to limit the use of self.outPath until NixOS/nix#4154 gets landed, not only because self.outPath prevents double mkFlake, but also because it would produce obscure error messages about infinite loop due to #148 in other use cases, including flake-parts's own tests.

@roberth
Copy link
Member

roberth commented Sep 3, 2023

I'd like to keep it simple, so I'm going for the root cause.

This would not be advisable for anything other than error messages,
because Nix does not commit to any semantics for that function.
The difference is that moduleLocation is "guaranteed" reliable data,
whereas errorLocation is the best choice for error messages in the core.

moduleLocation is suitable for the module key attribute.
errorLocation is best for the *ROOT* module _file attribute.

Initially I applied errorLocation in too many places. It is only needed
when the flake output attribute names are strict in it.
To avoid confusion, I'm not exposing errorLocation to the modules, until
we have a concrete use case for it.
As mentioned in the previous commit, we don't have a use case for
this in flake-parts yet, as far as I'm aware. It can be exposed
when we do have a concrete use case where it is needed.
@roberth
Copy link
Member

roberth commented Oct 13, 2023

root cause.

  • Blocked on

Rabbit hole too deep as that issue is itself blocked on another issue.
I think I've found an extended solution that's easier to use.

I'm not sure if it (automatically) solves all the problems you intended to solve, but it should solve

@Atry
Copy link
Contributor Author

Atry commented Oct 14, 2023

Looks good to me

@roberth roberth merged commit 25abf6c into hercules-ci:main Oct 29, 2023
2 checks passed
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.

2 participants