Run init scripts from the client distribution #5341
Conversation
5bdf0cc to
f10be5c
Compare
|
I found why it wasn't catching it - It was starting a new daemon and there was no assertion that the daemon is reused. I'll have a fixed test soon. |
f10be5c to
63da558
Compare
The recent change to start parameter serialization had omitted the Gradle home directory, leading to Gradle chosing the distribution that the daemon was started with over the distribution that the client wanted. This is now fixed. This also fixes another issue where the Gradle home was not passed on to nested builds like buildSrc, which lead to the same problem for them. Gradle#getGradleHomeDir will now correctly return the home dir that the client selected, not the home dir that the daemon was originally started with. The test veryfing all this has been fixed so that it asserts that the daemon was reused. Previously it was just creating new daemons and thus passing.
63da558 to
c4ce376
Compare
|
@big-guy Can I request your review on this since it's getting late in Europe? |
|
@oehme I got it, no worries. |
There was a problem hiding this comment.
@oehme there were some unit test failures around the changes to DefaultGradle, but looking at the TAPI test failures makes me think this has wider implications that changes other behavior.
I think it makes sense to have gradle.gradleHomeDir match the daemon installation. I think build scans captures this information somewhere.
| } | ||
|
|
||
| @Override | ||
| public File getGradleHomeDir() { |
There was a problem hiding this comment.
I think this changes the meaning of gradle.gradleHomeDir a bit...
Returns the Gradle home directory, if any. This directory is the directory containing the Gradle distribution executing this build.
When using the “Gradle Daemon”, this may not be the same Gradle distribution that the build was started with. If an existing daemon process is running that is deemed compatible (e.g. has the desired JVM characteristics) then this daemon may be used instead of starting a new process and it may have been started from a different “gradle home”.
I read that to mean that gradle.gradleHomeDir should always be the installation of the Gradle daemon that's running the build and not the client requested Gradle installation.
The TAPI failing tests point to that too:
https://builds.gradle.org/viewLog.html?buildId=12814254&tab=buildResultsDiv&buildTypeId=Gradle_Check_Quick_Java7_Oracle_Windows_toolingApi
This is saying that the first build starts a daemon with distro1. The second build, using distro2, uses the distro1 daemon. The build sees the init scripts from the right distribution, but gradle.gradleHomeDir is the installation directory of the daemon running the build.
There was a problem hiding this comment.
Makes sense. It just shows that the init script integ test was never working.
|
|
||
| then: | ||
| distro2Result.output.contains "from distro 2" | ||
| distro2Result.output.contains "runtime gradle home: ${distro2.absolutePath}" |
There was a problem hiding this comment.
based on the TAPI test, I think this was supposed to be looking for distro1.absolutePath.
|
Superseded by #5378 |
The recent change to start parameter serialization
had omitted the Gradle home directory, leading to
Gradle chosing the distribution that the daemon was
started with over the distribution that the client
wanted. This is now fixed.
This also fixes another issue where the Gradle home
was not passed on to nested builds like buildSrc,
which lead to the same problem for them.
Gradle#getGradleHomeDir will now correctly return
the home dir that the client selected, not the home
dir that the daemon was originally started with.
The test veryfing all this has been fixed so that
it asserts that the daemon was reused. Previously
it was just creating new daemons and thus passing.