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

Add compiler command crystal clear_cache #13553

Merged

Conversation

baseballlover723
Copy link
Contributor

@baseballlover723 baseballlover723 commented Jun 10, 2023

Seemed simple enough to implement so I took a crack at it. Working perhaps isn't the best, but I think is moderately clear enough. Didn't add any tests because I don't know enough about the compiler to know how to easily generate cache files.

Fixes #4730

@straight-shoota
Copy link
Member

For testing, you could just write a file to CacheDir.instance.dir, run the command and verify that the file is gone.
However, we don't really have much testing of compiler commands at all, so it should no be a requirement to add it for this one (it would be nice, though).

@baseballlover723
Copy link
Contributor Author

For testing, you could just write a file to CacheDir.instance.dir, run the command and verify that the file is gone. However, we don't really have much testing of compiler commands at all, so it should no be a requirement to add it for this one (it would be nice, though).

Added a spec for it. Not 100% sure if we actually want this spec or not, since it actually deletes the actual compiler cache, and I could imagine that there might be an issue with doing that during the tests. I'm thinking that it would be problematic if the compiler tests were ever run in parallel, since it could easily interfere with another test.

I briefly took a look at making a tempdir for the test and trying to use that instead of the actual compiler cache. Couldn't reassign the CacheDir singleton, and I'm not familiar enough with crystal to really know the best way to mock out a singleton. So if you have any guidance on that then I'd be happy to modify the spec so that it uses a tempdir instead of the actual compiler cache. Otherwise, I think this is alright for now (especially since the spec isn't really a requirement for the PR)

@Sija
Copy link
Contributor

Sija commented Jun 15, 2023

I briefly took a look at making a tempdir for the test and trying to use that instead of the actual compiler cache. Couldn't reassign the CacheDir singleton, and I'm not familiar enough with crystal to really know the best way to mock out a singleton. So if you have any guidance on that then I'd be happy to modify the spec so that it uses a tempdir instead of the actual compiler cache. Otherwise, I think this is alright for now (especially since the spec isn't really a requirement for the PR)

Closest to mocking that stdlib provides, is monkey-patching the CacheDir class, sth like:

class Crystal::CacheDir
  class_setter instance

  def initialize(@dir)
  end
end

This would allow you to swap the CacheDir instance in specs, preferably within around_each hook.

Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

Nice. Doing that much work for the spec really wasn't expected of you.
It could potentially pose a problem if we run specs concurrently. For that to work correctly, we'd need to run the command in complete isolation. But we don't do that right now, and there are probably other issues with that as well in the compiler specs.
So I'm happy to take it in. We'll see if there happen to be any issues with it.

@straight-shoota straight-shoota added this to the 1.9.0 milestone Jun 15, 2023
@straight-shoota
Copy link
Member

@baseballlover723 Please avoid force pushes to a PR, as requested in the the contributing guide. Especially, after it has been approved and milestoned.

@baseballlover723
Copy link
Contributor Author

baseballlover723 commented Jun 19, 2023

@baseballlover723 Please avoid force pushes to a PR, as requested in the the contributing guide. Especially, after it has been approved and milestoned.

My apologies, I didn't realize that that document existed. I'll refrain from force pushing in the future. Thanks for linking the document to me.

@straight-shoota straight-shoota changed the title added a compiler command to clear the compiler cache Add compiler command crystal clear_cache Jun 19, 2023
@straight-shoota straight-shoota merged commit a12dcf8 into crystal-lang:master Jun 19, 2023
50 checks passed
@baseballlover723 baseballlover723 deleted the pr/clean_compiler_cache branch June 19, 2023 19:40
Blacksmoke16 pushed a commit to Blacksmoke16/crystal that referenced this pull request Dec 11, 2023
Co-authored-by: Sijawusz Pur Rahnama <sija@sija.pl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

clearing compile-time cache via crystal
5 participants