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

Repoint the 'current' symlink even for valid VTs. #5375

Merged
merged 1 commit into from Jan 23, 2018

Conversation

Projects
None yet
3 participants
@benjyw
Copy link
Contributor

benjyw commented Jan 23, 2018

Previously we only repointed it for invalid ones, which meant
that the "current" link could be stale if you had two valid versions
of a target and were switching between them (e.g., by reverting
a change).

This change also renames results_dir_path() to _results_dir_path(),
to emphasize that it's not for outside consumption.

It also removes a superfluous arg to a test helper method, fixes
a bug in a test where we used assertTrue instead of assertEquals,
and removes a superfluous 'or False' from a boolean condition.

Repoint the 'current' symlink even for valid VTs.
Previously we only repointed it for invalid ones, which meant
that the "current" link could be stale if you had two valid versions
of a target and were switching between them (e.g., by reverting
a change).

This change also renames results_dir_path() to _results_dir_path(),
to emphasize that it's not for outside consumption.

It also removes a superfluous arg to a test helper method, fixes
a bug in a test where we used assertTrue instead of assertEquals,
and removes a superfluous 'or False' from a boolean condition.

@benjyw benjyw requested review from stuhood and baroquebobcat Jan 23, 2018

@mateor

mateor approved these changes Jan 23, 2018

Copy link
Member

mateor left a comment

I was surprised to see that the results_dir_path was made public. Looks like that happened here but I am unsure why.

I have no objection, but I am also not sure that I follow the motivating case. It seems like the previous_cache_key should have changed when the bad change landed and so the revert would naturally result in an invalid task and a wiped results_dir.

If I hit this case personally, I would wonder if I found a hole in the target's fingerprinting strategy. But I am willing to trust you are onto something.

@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented Jan 23, 2018

The test proves that something is up (I verified that it failed without the change)...

The specific case I had was when going back and forth between two different values of a fingerprinted option. The option fingerprint affects the cache_key.id, apparently, so the previous_cache_key read from disk is the same as the computed one.

@benjyw benjyw merged commit e0dd4b4 into pantsbuild:master Jan 23, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@benjyw benjyw deleted the benjyw:fix_current_links branch Jan 23, 2018

@stuhood
Copy link
Member

stuhood left a comment

Thanks, looks good.

stuhood added a commit that referenced this pull request Jan 24, 2018

Repoint the 'current' symlink even for valid VTs. (#5375)
Previously we only repointed it for invalid ones, which meant
that the "current" link could be stale if you had two valid versions
of a target and were switching between them (e.g., by reverting
a change).

This change also renames results_dir_path() to _results_dir_path(),
to emphasize that it's not for outside consumption.

It also removes a superfluous arg to a test helper method, fixes
a bug in a test where we used assertTrue instead of assertEquals,
and removes a superfluous 'or False' from a boolean condition.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment