Support Gradle 7.4 configuration caching #211
Merged
+151
−107
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nhoughto
commented
Jan 27, 2022
| @@ -47,10 +46,7 @@ class YarnProxy_integTest extends AbstractIntegTest { | |||
| request.removeHeader("host") | |||
| def targetHost = "registry.npmjs.org" | |||
| request.withHeader("host", targetHost) | |||
| if (secure) { | |||
There was a problem hiding this comment.
wasn't really sure what the deal with this was? npm registry is always doing to be secure upstream, you get a redirect if you connect over port 80.
There was a problem hiding this comment.
Since Java supports both a HTTP and a HTTPS proxy we used to test that both worked, with HTTP support dropped I gave a brief try fixing the tests but got unpredictable results and instead disabled the HTTP tests (there's a separate issue for the proxy tests)
nhoughto
commented
Jan 27, 2022
| .withPath("/case") | ||
| .withHeader("Host", "registry.npmjs.org:${port}"), | ||
| exactly(1)) | ||
| proxyMockServer.verify(request() |
There was a problem hiding this comment.
not sure if this assertion was important or not? can't see why it would be, failed on my local either way.
There was a problem hiding this comment.
There's #195 for the broken tests, when we test the proxy support we install the case module and then have this assert verify that the request actually went through the proxy (which in our case is the mockserver proxying the request)
nhoughto
commented
Jan 27, 2022
| // Added so that we can test osArch on Windows and on non-arm systems | ||
| if (name == "uname") execute("uname", "-m") | ||
| else error("Unable to find a value for property [$name].") | ||
| } | ||
|
|
||
| open fun getSystemProperty(name: String): String? { |
There was a problem hiding this comment.
might be a more kotlin-y way to do this 🤷♂️
|
I just need to look through the test changes, but so far it looks great 👍 I was pleasantly surprised to see that Gradle 7.4 supports Automatic detection of environment variables, system properties and Gradle properties used at configuration time Especially
(sorry, cat walked over keyboard) |
|
Looks good 👍 I've copied this branch into the repository and switched to 7.4-rc2 that released today, going to have a look tomorrow to see if the next release will be a major or minor and do some manual testing to make sure the proxy support still works If all looks good I'll merge this to master, move the remainders in the current milestone to the top of the next milestone and push a release |
|
So you’ve got it from here? Don’t need me to do anything else?
|
|
Yup, got all that's needed, I'll publish the release tonight |
|
ok great, then i can stop maintaining my private fork 👍 |
|
Hi! I'm getting the following error with a bulid restored from the configuration cache with gradle-node-plugin version 3.2.0. Failed to stop service 'gradle-node-modules'.
Do we need a separate MR for this? |
|
It'd be great if you could create a separate issue for it with a small project that shows it being triggered I'm still recovering from a fever so there'll be some days before I can have a proper look at things again |
|
This isn't related to this gradle plugin i don't think, the error in question is related to a build service 'gradle-node-modules' which this plugin doesn't create or register (i dont think?), seems to be from the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
propsvariable inPlatformHelperto avoid failures in 7.4