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

Fix copying tasty files on incremental compilation #1701

Merged
merged 1 commit into from
Mar 15, 2022

Conversation

tgodzik
Copy link
Contributor

@tgodzik tgodzik commented Mar 14, 2022

Previously, we removed both generatedClassFile and rebasedClassFile from invalidations, but we only did it for rebased product files such as .tasty. Now we do it for both

I am not 100% sure why sometimes invalidations happen for rebased and sometimes for generated files, but that can happen according to the comment above.

What happened here is that the generated files existed, but the rebased one was in the invalidation list, so later on we would skip it.

This was causing the issues in scalameta/metals#3706

Previously, we removed both generatedClassFile and rebasedClassFile from invalidations, but we only did it for rebased product files such as `.tasty`. Now we do it for both

I am not 100% sure why sometimes invalidations happen for rebased and sometimes for generated files, but that can happen according to the comment above.

What happened here is that the generated files existed, but the rebased one was in the invalidation list, so later on we would skip it.
@tgodzik tgodzik requested review from adpi2 and kpodsiad March 14, 2022 15:08
@kpodsiad
Copy link
Collaborator

kpodsiad commented Mar 14, 2022

Question for Zinc newbies, what are generatedClassFile and rebasedClassFile? (generated I think is obvious, but the second one isn't)

@tgodzik
Copy link
Contributor Author

tgodzik commented Mar 14, 2022

Question for Zinc newbies, what are generatedClassFile and rebasedClassFile? (generated I think is obvious, but the second one isn't)

I think the rebased one is the on where we put copy of files for the client.

@tgodzik
Copy link
Contributor Author

tgodzik commented Mar 14, 2022

Actually, now I think of it. Rebased might be a new directory and we might always be compiling to a new directory but using the artifacts from the old one. 🤔

@kpodsiad
Copy link
Collaborator

@tgodzik ELI5: what is this PR about and what are invalidations. 🙏 Incremental compilation isn't my strong side yet :D

@tgodzik
Copy link
Contributor Author

tgodzik commented Mar 15, 2022

@tgodzik ELI5: what is this PR about and what are invalidations. pray Incremental compilation isn't my strong side yet :D

Ach sorry, each time anything changes in the code the files get invalidated (meaning they need recompilation) and when those get recompiled we might need to recompile some additional files etc.

Copy link
Member

@adpi2 adpi2 left a comment

Choose a reason for hiding this comment

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

While I am not sure to understand why invalidation can happen in both paths, it makes sense to remove all the products of the invalidated class files.

@tgodzik
Copy link
Contributor Author

tgodzik commented Mar 15, 2022

I will go ahead and merge this since from my understanding this is the safest choice.

I think the main reason for this is that we change output directory on each compilation, which is why there might be different paths for invalidated files and new generated ones.

@tgodzik tgodzik merged commit 7848b60 into scalacenter:main Mar 15, 2022
@tgodzik tgodzik deleted the fix-copy-testy branch March 15, 2022 12:57
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.

3 participants