SwiftGen / SwiftGenPlugin

SwiftGen plugin for SPM
MIT License
95 stars 63 forks source link

Make sure that `swiftgen` can use cache correctly. #7

Closed cheif closed 1 year ago

cheif commented 2 years ago

This remove the lines that always emptied pluginWorkDirectory before running swiftgen. That logic results in swiftgen having to re-generate the files on every build, which in turn leads to the compiler seeing new timestamps for the generated files, leading in a lot of re-compilation happening.

I don't fully understand why the lines were there originally, but from what I've read about SPM plugins we should try to use pluginWorkDirectory as a cache, which will be the case with this change.

djbe commented 2 years ago

As I remember it, the line is there because if the user removes or changes a part of their config, such as changing the name of an output file, then we don't know about it and a dangling file will be left in that folder.

This isn't much of an issue for normal SwiftGen use, the user controls the config file and the project file, it's their responsibility.

In the case of SwiftGenPlugin, it's more muddled. All generated files placed in that directory will be compiled with the user's project. So in the case of our rename example, we'll have duplicate output for a command: the old output file and the new one, with the same code, compiled into the user's project.

A better improvement would be keeping track of the old config & the new one, compare the old one's output paths with the new one's, and delete any dangling files. Currently the plugin is NOT aware of any of this, it ignores the actual contents of the config file(s).

cheif commented 2 years ago

Thanks for a quick reply @djbe!

I see, that case makes sense now when you explained it, and I realise that my current patch wont work.

My understanding about SPM (and I guess the swift build-system in general) is that we should try to rely on timestamps some info here, do you think it would make sense to do that here as well?

I see two different solutions:

  1. Check the timestamp of swiftgen.yml(s), and compare against pluginWorkDirectory, and if the config has changed we empty the directory as right now, but otherwise we skip doing that.
  2. Try to compare timestamps of all input-files to pluginWorkDirectory, and if nothing has changed we don't run swiftgen at all (returning an empty array?). This would mean that we increase the performance a bit as well by not running swiftgen uneccessarily.

I think I'd prefer 2., at least in the long term. Since this is a plugin that will be run a lot of times I think it makes sense to put some extra effort into making it run as fast as possible. I'm not sure about how we'd go about finding all the input files though, and I guess that would mean this plugin would need to be more aware of swiftgen internals?

So, unless you have other thoughts, I think I'd go with 1., I should be able to make that change to this PR later today 😄

djbe commented 2 years ago

I was thinking more along the lines of keeping a copy of the config somewhere in the pluginWorkDirectory, hash the original & the copy, and if the hashes are different, nuke the output files.

Note that the mention of timestamps in your link is that we shouldn't change the timestamps if possible (which we normally do in SwiftGen, and will work once we don't nuke files on every run).

cheif commented 2 years ago

Ok, so now I've updated the code a bit, following your suggestion the best way I could.

I couldn't come up with a good way of storing the swiftgen.yml files in the cache, since we support having 2 (or maybe more) of those, and then we'd need to keep track of the paths to the respective files etc. So I opted for just hashing them all and then storing that checksum in the cache, I think that should be equally good?

I also had to change the command itself to output files to .pluginWorkDirectory + "/output", otherwise you'll get warnings about having the dangling swiftgen-checksums files in the directory specified as the outputFilesDirectory.

So, please have a look to see if this solution seems more reasonable 😄

cheif commented 2 years ago

Hi @djbe

Just curious if you have any more feedback on this PR, it would be nice to get it in so that we can start using swiftgen through the plugin! 😊