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

Enable lazy/sparse allocation of generation symlinks #538

Merged
merged 3 commits into from
May 21, 2015

Conversation

ctheune
Copy link
Contributor

@ctheune ctheune commented May 18, 2015

This avoids creating new generations if a generation already exists.

Alternatively or additionally I propose a mode where only the last generation will be sparse.

Would love some feedback and would be happy to implement the alternative strategy if this is too aggressive ... :)

new generations if a generation already exists.

Alternatively or additionally I propose a mode where only the *last* generation will be sparse.
@edolstra
Copy link
Member

Doesn't this break --rollback? Since generation "current - 1" is no longer necessarily the previous generation...

@ctheune
Copy link
Contributor Author

ctheune commented May 18, 2015

Ah. Interesting.

The more aggressive option that I just implemented would break this.

The less aggressive option would change the semantics in a way that could be argued over. If only the newest generation would be stopped from creating new identical generations over and over again then rollback would get the semantic of "change to the generation before the last change" instead of "change to the same generation that we just created three times anyway".

Glad you caught this: I had a nagging feeling that the current implementation has some edge case that I wasn't aware of. How about the other option?

@edolstra
Copy link
Member

The other option sounds good. It also has a slight problem, namely that it creates the possibility that the combination of nix-env -i ...; nix-env --rollback is not a no-op, if nix-env -i is a no-op, but that's actually already the case (since the current generation might not be the highest number due to previous rollbacks).

@ctheune
Copy link
Contributor Author

ctheune commented May 18, 2015

Alright. So if I implement the other version you'd pull that?

If you're worried about the other interaction then we could make this an option, but I'd rather not create more optional complexity ... ;)

@edolstra
Copy link
Member

No, let's not make it an option.

@copumpkin
Copy link
Member

This will make NixOS/nixpkgs#7369 a lot more sensible.

@ctheune
Copy link
Contributor Author

ctheune commented May 18, 2015

That actually is a use-case that I'm aiming for. I'll subscribe to that issue.

* only the last generation can be lazy
* depend on the '--lazy-generation' flag to be set
@ctheune
Copy link
Contributor Author

ctheune commented May 19, 2015

Alright. How about this? I'll be happy to run through the documentation and update it if you're happy so far. Otherwise, let me know what needs changing. :)

@edolstra
Copy link
Member

Hm, so you added an option after all. If we're going to have an option, I think it would be preferable to have "lazy" as the default.

@ctheune
Copy link
Contributor Author

ctheune commented May 20, 2015

Hah. This shows its better to talk about code. :)

I misunderstood you asking for an option - I thought that's what you wanted. I'm happy to rip it out again. I'd prefer this to be the way it is.

Anyway, made me regain and exercise some more C++ knowledge, though. No hard feelings.

I'll update later today.

@ctheune
Copy link
Contributor Author

ctheune commented May 20, 2015

Ah. And now that I re-read your comment. For some reason I kept staring at it and it kept telling me "lets make it an option". I swear I re-read this comment about a half bazillion times to make sure that's what you want. ;)

@ctheune
Copy link
Contributor Author

ctheune commented May 20, 2015

Alright. Thanks to my back and forth ... we now have a quite small patch. Is there a place in the documentation that would be worthwhile to update?

@edolstra edolstra merged commit 12a8888 into NixOS:master May 21, 2015
@edolstra
Copy link
Member

Merged, thanks!

@vcunat
Copy link
Member

vcunat commented Jun 7, 2015

This change will get included in 15.06, right? (Meaning the default nix version in there.) If so, we should mention the change of semantics in release notes, OK?

zolodev pushed a commit to zolodev/nix that referenced this pull request Jan 1, 2024
Set the structure of the tutorials section
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