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

gitPublishCopy doesn't get implicit dependencies from gitPublish.contents anymore #41

Closed
ajoberstar opened this issue Oct 28, 2017 · 6 comments · Fixed by #45
Closed
Labels
Milestone

Comments

@ajoberstar
Copy link
Owner

In 0.3.2, a fix to #35 was added that stops using a Copy task for gitPublishCopy. This avoids some issues with an inconsistent use of declared inputs/outputs. However, now that we're using a project.copy call with the gitPublish.contents CopySpec, we don't get the implicit dependencies we used to.

This means something like this:

task createOutput { // outputs some stuff }

gitPublish {
  contents {
    from createOutput
  }
}

Results in:

./gradlew gitPublishPush -m
:gitPublishReset
:gitPublishCopy 
:gitPublishCommit
:gitPublishPush

Instead of:

./gradlew gitPublishPush -m
:gitPublishReset
:createOutput
:gitPublishCopy 
:gitPublishCommit
:gitPublishPush
@ajoberstar ajoberstar added the bug label Oct 28, 2017
@ajoberstar
Copy link
Owner Author

@wolfs, would you mind providing some feedback here?

Since CopySpec doesn't implement/extend anything that Task#dependsOn accepts, I'm not sure how to (or if I can) get implicit dependencies to work again after the switch away from the Copy task.

@wolfs
Copy link

wolfs commented Oct 29, 2017

The easiest way is to add the contents of the copy spec as an input. This can be done for example by:

inputs.files {
  contents.buildRootResolver().getAllSource()
}.withPropertyName('contents').skipWhenEmpty() 

Sadly, buildRootResolver() is not part of the public API :(

@ajoberstar
Copy link
Owner Author

OK, I'll have to think about whether there's another way to handle input/outputs or just documenting that they'll need to be added explicitly. Thanks for sending that over.

@wolfs
Copy link

wolfs commented Nov 13, 2017

I thought some more about the problems with not declaring outputs of the tasks vs. declaring outputs of the tasks and I think there would be less surprises for the users if all tasks for this set of plugins would declare the outputs.
Like we saw here we run into some difficult to understand problems and cannot leverage some of Gradle's features. A user of the plugin could define another copy task into the repository, inadvertently causing the destination files to be deleted.
The only downside is that we currently pay a price for taking a snapshot of the git repository.
WDYT?

@ajoberstar
Copy link
Owner Author

Thanks for thinking more about this.

Yeah, I agree on the inputs/outputs posing fewer surprises. Some of the finer points on the inputs/outputs aren't clear to me, somewhat related to your comments on #39:

I guess you also should do outputs.upToDateWhen { false } to make sure the task is always executed. Given that is has no inputs that is what would happen if the output directory contents would not change. Alternatively, you could also add the inputs to the task (git commit hash, branch, remote?) if that is possible. Same goes for the other tasks in here.
In the future, we may provide you with a way to declare that there should not be any up-to-date checks for a task, so you don't pay the performance penalty of checking all the files in the repository for changes.

I should be able to identify upToDateWhen checks for each of the tasks. I'm less sure what to do for inputs. This would probably include writing "real" tasks (which is probably beneficial anyway), which might do the trick.

Also, does it make sense for all of these tasks to use the repo directory as an output? Or should it just be the reset (which sets it up in the first place) and the copy? What does the outputs declaration do when many tasks change small pieces of the same directory?

@wolfs
Copy link

wolfs commented Dec 24, 2017

@ajoberstar Sorry for taking so long to come back to you.
I would suggest you start with using upToDateWhen { false } on all of the tasks and declare the repo directory as an output. It would be beneficial to convert you adhoc tasks to task classes, so you could even start to write some testkit tests for them. Declaring the outputs would also be simpler if you use custom task classes, the plugin could set the property by using lazy properties to wire the tasks to the extension.
You may want to look at the Implementing Gradle plugins guide, especially at the chapter about writing and using custom task types and maybe the chapter on how to benefit from incremental tasks.
Hope that helps.

ajoberstar added a commit that referenced this issue Apr 20, 2018
Implement real task classes instead of ad-hoc ones. This allows proper
declaration of inputs/outputs. Generally, the intent is to ensure Gradle
knows the repo directory is managed by these tasks. This allows us to
restore support for implicit task dependencies when someone uses the
contents copy spec:

  task createOutput { // output some stuff }

  gitPublish {
    contents {
      from createOutput
    }
  }

This drops support for Gradle < 4.3 due to requirement of Provider APIs
for lazy configuration.

This fixes #41.
@ajoberstar ajoberstar added this to the 0.4.0 milestone Apr 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants