Closed adammurdoch closed 5 years ago
So, the trick here is in determining when to validate the targets. Some options:
afterEvaluate
hook. The downside of this is that it fires on every invocation, whether assemble
gets invoked or not. It also potentially opens us up to an ordering problem with other afterEvaluate
hooks. assemble
task. The problem with this is that in a multi-project build, tasks in other projects could run first causing the failure to occur late.taskGraph.whenReady
hook. This allows us to only fire when assemble is called and fail fast when we know that a component does not have any buildable binaries. The downside of this is that to know whether assemble
is invoked, we need the task path, which is only directly available after the task is realized. To avoid realizing assemble (and creating a chain reaction of realization on its task dependencies) we'd need to reconstruct the task path separately, which is totally doable, but seems a little awkward.I think we want to avoid doing the check unless we know that assemble
has been invoked. There is the valid use case where I might have a multi-project build with conditional dependencies on operating-system-specific libraries (see https://github.com/gradle/native-samples/tree/master/cpp/operating-system-specific-dependencies). In that case, it's totally valid that if I'm building on Linux, the windows dependency project would not have any buildable libraries, so we would only want to fail if the windows dependency was "assembled", otherwise other projects in the build should succeed.
Given that, I feel like the taskGraph.whenReady
hook is the way to go. We might also consider allowing TaskProvider
to track/provide the task path in addition to the task name. We should also do whatever validation we do for assemble
for test
as it also makes sense that if I try to run the tests of a component on an operating system that is not targeted, this should cause a failure rather than silently succeeding.
Thoughts?
Another option, is to do the check when the dependencies of the assemble
task are calculated. This feels like a better fit, as the dependencies for assemble
are not defined when the binaries cannot be built so querying them should fail.
Ah, that's a good idea - I'll give that a try...
@adammurdoch I'm still concerned about the usability of this behavior in multi-project builds.
For single project builds, I get that it would be very friendly to fail gradle assemble
with a meaningful message.
For multi-project builds, I get that it would be nice to fail if gradle assemble
can't build anything.
At the very least, Gradle shouldn't surprise you by doing nothing if you've asked it to do something.
But for most multi-project builds, I think the reality is going to be that there will always be a subset of the build that is not buildable because it builds for a different platform. Gary's PR makes it so running gradle [check|assemble|build]
from the root of the build is very surprising in a mixed platform build.
I think it comes down to interpretting intent. In a single project, gradle assemble
means assemble this project. In a multi-project, gradle app:assemble
means assemble the app project. In both of those cases, we can fail because we know the user wants to do something that can't be done.
In a multi-project, gradle assemble
could mean "assemble everything" or it could mean "assemble everything that's buildable". From our CLI task name resolving, we interpret gradle assemble
to mean "assemble everything", but non-buildable projects have nothing to assemble, so the current behavior looks like "assemble everything that's buildable".
I would argue that that's the more useful behavior and not "assemble everything". The two real world examples we have for the new plugins have exactly this scenario (some subprojects build for only Linux and others build for macOS and Linux). If we had the new behavior for those projects, you would no longer be able to use the typical lifecycle tasks. You may be forced to create new assembleBuildableThings
tasks for only the buildable projects or come up with some other way to assemble on the things that should be built. I worry the new behavior will leave the impression that gradle [check|assemble|build]
shouldn't be used or that Gradle isn't smart enough to figure out what "should" be built.
You can see this directly in our operating-system-specific-dependencies
sample. This is no longer buildable with the PR above, unless you know which subprojects can build on your host.
There's another problem with the PR in that if you have multiple things that could be assembled in a subproject and only one of those is from the native plugin, you now can't build the other part.
I think the downside of keeping the existing behavior is exactly what you raised. If the project shouldn't be built for my current host, but you explicitly ask for it, it's surprising that this is successful. Is there something else we could do here?
check
, assemble
, build
unless something is buildable? This is probably hard to do and share the lifecycle base plugin.WDYT?
The main issue I see with keeping the current behavior is that the build "succeeds" but doesn't actually do anything which may be confusing to the user. Another option might be that instead of failing the build, we just output a warning that the component does not target the current machine and that nothing will be built, but still succeed. Then at least it's clear to the user that nothing happened and why.
We should be clear to distinguish between "does not target the current machine" and "targets but is not buildable on the current machine". This issue is only to give reasonable feedback when I accidentally run a build on the wrong machine, that is, where there are no native components that target the current machine defined anywhere the build.
I think "reasonable feedback" in this case should be a build failure, rather than just a warning, so that this is visible to tooling as well.
I wouldn't try to fix every usability issue at once. The case where I have some projects in the build that target the current machine and some that don't is not really in scope for this issue. We can have the build fail in this case, or we can have it ignore components that don't target the current machine. I think it's ok for now to fail, because it's much simpler to implement, it's easy for people to add their own workflow tasks, and improves the more common case. I'd wait and see if the mixed case becomes a problem for developers and address it if so.
Regardless of all this, we should never ignore a component that targets the current machine. We should fail if none of its variants are buildable.
I merged Gary's PR and switched the failure to a warning based on what Adam and I discussed. This keeps our multi-project build working
When trying to build everything in our OS-specific sample on macOS, you now see:
'main' component in project ':winConsole' does not target this operating system.
If you try to build a single-project build, you see something similar.
gradle assemble
should fail if I can not build something that I can run on the current host machine. For now, we can assume that "runs on current host" == "targets current host". So, for example, when I try to build a C++ application that targets Windows on Linux, the this should fail with an error telling me this. The error should be different to that produced when the component targets the host machine but there are no tools available to build the binaries.