Skip to content
This repository was archived by the owner on Dec 7, 2024. It is now read-only.

Add a source directory for generated files #10

Merged
merged 2 commits into from
Jul 16, 2021

Conversation

jpenilla
Copy link
Contributor

Fixes an issue where when attempting to clean build, sometimes java.io.IOException: Unable to delete directory would fail the build.

@stephan-gh
Copy link
Member

stephan-gh commented May 28, 2021

You seem to be doing several things at once here that don't fully make sense to me. I don't think it's right to declare an @OutputDirectory, the task generates a single file so an output directory is a bit overkill IMO. Can you provide an example build that allows reproducing your issue? Perhaps there is a cleaner way to fix this.

@jpenilla
Copy link
Contributor Author

https://paste.gg/p/anonymous/b15485380a4d443e88b257d00c7bb414 This is the issue I have been having, not sure exactly what causes it, but when running ./gradlew clean build on these two projects: 1, 2 it happens about 30-50% of the time, and these changes solved this. I wasn't able to reproduce with a single-module project, so it's possible it might something to do with multi-module setups?

I do have an ugly workaround, which is just configuring the yml generation task with mustRunAfter(clean), but I think the solution proposed in this PR is cleaner. Adding our yml though a generated sources directory seems to coerce Gradle into ordering the tasks more correctly without explicitly declaring task ordering.

I came to this solution after noticing that the SpongeGradle plugin-development plugin, which generates a single json file in a similar way to plugin-yml, does not have this issue, and saw this is how they were including their generated files.

@stephan-gh
Copy link
Member

Hm. I tried for many, many times (with your projects) but I didn't manage to reproduce this for some reason... I wonder if this is related to issue 2488 on https://github.com/gradle/gradle (don't want a backlink here), but you don't seem to have any dependencies on the clean task either.

@jpenilla
Copy link
Contributor Author

Tested on my laptop with macOS, and can't reproduce there, but happens consistently on my Arch Linux system.

@broccolai
Copy link

Also happens consistently for me with those projects and my own on Void Linux

@zml2008
Copy link

zml2008 commented May 30, 2021

I'm not entirely sure why Gradle does this, but FS ops can be strange at times.

The reasoning behind OutputDirectory in SG iirc was originally because a SourceSet's resources are a SourceDirectorySet, which forces inputs to be directories. The alternative of going through processResources works, sorta, but imo generated resources should not participate in whatever processing is applied to user-provided resources (adding extra resource directories also allows the generated resources to be picked up by IDE run configurations, which is a nice bonus.)

SpongeGradle's current implementation was last tweaked to comply with Gradle 7's implicit dependency validation, which it looks like jmp's solution here is also compliant with.

@stephan-gh
Copy link
Member

but happens consistently on my Arch Linux system

Weird, I'm also testing on Arch Linux and cannot reproduce this. :(

@stephan-gh
Copy link
Member

@zml2008

The alternative of going through processResources works, sorta, but imo generated resources should not participate in whatever processing is applied to user-provided resources

The input for processResources is SourceSet.getResources() (i.e. the SourceDirectorySet where you call the srcDir(...)) so your generated resources will still go through the processing.

(adding extra resource directories also allows the generated resources to be picked up by IDE run configurations, which is a nice bonus.)

I'm not sure I understand this correctly, what does this mean? When setting up an IDE run configuration, your approach adds it to the runtime classpath and mine doesn't?

SpongeGradle's current implementation was last tweaked to comply with Gradle 7's implicit dependency validation, which it looks like jmp's solution here is also compliant with.

plugin-yml works fine on Gradle 7 since the last update, so I think my approach also passes the validation? Anyway, I'm not opposed to change it to an output directory if that provides actual advantages (like the IDE stuff you mentioned?).

As I mentioned in my first comment, my main concern with the changes in this PR as-is is that it changes several things at once that are not strictly related to each other, so it's not entirely clear why it actually fixes that issue with the clean task.

I suspect the cause of the problems with the clean task is the use of temporaryDir in GeneratePluginDescription. Unfortunately, calling getTemporaryDir() on a Task does not just return a temporary dir, but also creates it. What likely happens is that Gradle evaluates the output files of the generateBukkitPluginDescription task while the clean task is running. So suddenly the temporary directory is created at the same time the clean task attempts to delete the build directory.

If there is really an advantage of adding a directory to SourceSet.getResources() instead of to processResources it might be enough to make the commit message a bit more clear with the above explanation.

resources using processResources

This allows for IDEs to recognize the generated resources.

This also fixes an issue where sometimes running `clean build`
would cause 'java.io.IOException: Unable to delete directory',
although this fix may also be due to the new approach no longer
creating a temporary directory.
@stephan-gh
Copy link
Member

@zml2008 Still kind of curious about the advantage of doing resources.srcDir(...). Can you elaborate a bit how this helps with IDE integration?

@stephan-gh stephan-gh merged commit 5e1f0bc into Minecrell:master Jul 16, 2021
@stephan-gh stephan-gh mentioned this pull request Jul 16, 2021
rainbowdashlabs pushed a commit to rainbowdashlabs/plugin-yml that referenced this pull request Oct 31, 2024
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants