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

Delay invalidating cache and gcroots until print-dev-env call succeeds #420

Merged
merged 3 commits into from
Nov 29, 2023

Conversation

bbenne10
Copy link
Contributor

@bbenne10 bbenne10 commented Nov 27, 2023

We currently eagerly invalidate the gcroots and old profile rc, assuming that the devshell is in a usable state.
If this assumption does not hold,
we can invalidate a working state for a broken one.

Here we just delay calling _nix_clean_old_gcroots
until we know that we're in a usable state.

In the case that the flake is in an unusable state, this simply reuses the latest working state.

This should address #412.

@bbenne10 bbenne10 mentioned this pull request Nov 27, 2023
@bbenne10 bbenne10 requested a review from Mic92 November 27, 2023 15:06
@bbenne10
Copy link
Contributor Author

I haven't looked into test failures, but will when I find a moment.

@bbenne10 bbenne10 self-assigned this Nov 27, 2023
direnvrc Outdated Show resolved Hide resolved
@Mic92
Copy link
Member

Mic92 commented Nov 29, 2023

@mergify queue

Copy link
Contributor

mergify bot commented Nov 29, 2023

queue

🛑 The pull request has been removed from the queue default

The queue conditions cannot be satisfied due to failing checks.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

@Mic92
Copy link
Member

Mic92 commented Nov 29, 2023

@bbenne10 I get the same errors locally. Presumably only present after doing the rebase?

We currently eagerly invalidate the gcroots and old profile rc,
assuming that the devshell is in a usable state.
If this assumption does not hold,
we can invalidate a working state for a broken one.

Here we just delay calling _nix_clean_old_gcroots
until we know that we're in a usable state.

In the case that the flake is in an unusable state,
this simply reuses the newest working state.

This should address #412.
@bbenne10
Copy link
Contributor Author

I am starting to believe that there's something going wrong with the interaction between the creation of the tmp_profile_rc and the _nix_add_gcroot call which causes direnv exec to exit non-zero. I haven't personally modified this behavior in this MR so I think we've got a longer standing bug, maybe?

I'm looking into it a bit more deeply now.

@bbenne10
Copy link
Contributor Author

Of course, I post and then figure out the bug.

_nix_clean_old_gcroots removes $tmp_profile. Moving it where I did causes it to remove $tmp_profile in between populating $tmp_profile_rc and the subsequent call to nix build (as part of _nix_add_gcroot), which causes the call to nix build to fail and thus the direnv exec call exits 1. I'll poke around to see how I can resolve this.

@bbenne10
Copy link
Contributor Author

There we go. Tests are passing now.

I have rebased on #427 to keep in line with the logic and logging changes in there. We should get that merged first.

@Mic92
Copy link
Member

Mic92 commented Nov 29, 2023

@mergify queue

Copy link
Contributor

mergify bot commented Nov 29, 2023

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at adeced7

@mergify mergify bot merged commit adeced7 into master Nov 29, 2023
29 checks passed
@Mic92 Mic92 deleted the delay_invalidating_cache branch November 29, 2023 22:03
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.

None yet

2 participants