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.

Sign up for GitHub

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

Jump to bottom

Pass auxiliary class files to zinc so they are deleted together #3072

Merged
merged 13 commits into from
Mar 11, 2024
Merged

Pass auxiliary class files to zinc so they are deleted together #3072

merged 13 commits into from
Mar 11, 2024

Conversation

lolgab
Copy link
Member

@lolgab lolgab commented Mar 9, 2024 •
edited
Loading

Fixes #3068
*.tasty, *.nir and *.sjsir files were not cleaned up by zinc when the companion class files were deleted.
This passes the right AuxiliaryClassFiles to zinc.

Pull Request: #3072

@lolgab lolgab marked this pull request as ready for review March 9, 2024 14:28
@lolgab lolgab requested a review from lefou March 9, 2024 14:28
@lolgab
Pass auxiliary class files to zinc so they are deleted together
611a7e9 
Fixes com-lihaoyi#3068
`*.tasty`, `*.nir` and `*.sjsir` files were not cleaned up
by zinc when the companion class files were deleted.
This passes the right `AuxiliaryClassFiles` to zinc
@lolgab lolgab force-pushed the delete-auxiliary-class-files branch from 793ad6a to 611a7e9 Compare March 9, 2024 14:38
@lolgab lolgab mentioned this pull request Mar 9, 2024
Closed
lefou
lefou reviewed Mar 9, 2024
View reviewed changes
Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

I think the tests should also assert the success or failure of the compile task, since we have a strong expectation, that the second run should fail.

@lolgab
Copy link
Member Author

lolgab commented Mar 10, 2024

I think the tests should also assert the success or failure of the compile task, since we have a strong expectation, that the second run should fail.

To not make other tests more complex since they used a single file, I added a separate test that writes another source file in place and asserts that the compilation first succeeds and then fails after deleting the foo.scala file.

@lolgab lolgab requested a review from lefou March 10, 2024 11:36
@lolgab lolgab requested review from lefou and removed request for lefou March 11, 2024 10:47
lefou
lefou reviewed Mar 11, 2024
View reviewed changes
Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

Looks good to me. Just two small doc improvements.

scalalib/src/mill/scalalib/JavaModule.scala Outdated Show resolved Hide resolved
scalalib/api/src/mill/scalalib/api/ZincWorkerApi.scala Outdated Show resolved Hide resolved
@lolgab lolgab force-pushed the delete-auxiliary-class-files branch from 1604252 to 52e6675 Compare March 11, 2024 19:00
@lolgab lolgab requested a review from lefou March 11, 2024 20:13
lefou
lefou approved these changes Mar 11, 2024
View reviewed changes
Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thank you!

@lefou lefou mentioned this pull request Mar 11, 2024
Closed
@lolgab lolgab merged commit c7c6fcd into com-lihaoyi:main Mar 11, 2024
38 checks passed
@lolgab lolgab deleted the delete-auxiliary-class-files branch March 11, 2024 22:39
@lefou lefou added this to the 0.11.8 milestone Mar 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adpi2 adpi2 adpi2 left review comments

@lefou lefou lefou approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.11.8
Development

Successfully merging this pull request may close these issues.

Tasty files are not deleted
3 participants
@lolgab @lefou @adpi2