-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Converting DependencyLicensesTask and UpdateShasTask to java #35231
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
Conversation
|
@elasticmachine test this please |
|
Pinging @elastic/es-core-infra |
alpar-t
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thansk for your contribution @Megamiun .
I left some comments. Still have to look at the tests.
Perhaps would be good to add a testkit based test to make sure that the up-to-date checks work as they should. Calling the task twice should result in the second time being up-to-date
buildSrc/src/main/java/org/elasticsearch/gradle/precommit/DependencyLicensesTask.java
Outdated
Show resolved
Hide resolved
buildSrc/src/main/java/org/elasticsearch/gradle/precommit/DependencyLicensesTask.java
Outdated
Show resolved
Hide resolved
buildSrc/src/main/java/org/elasticsearch/gradle/precommit/DependencyLicensesTask.java
Outdated
Show resolved
Hide resolved
buildSrc/src/main/java/org/elasticsearch/gradle/precommit/DependencyLicensesTask.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should expose this differently, but that can go into a separate PR.
Assigning this from configurations makes it way too easy to resolve the configurations too early.
We should get the dependencies on project like it's being assigned in BuildPlugin and offer ways to filter them, like having a List<Predicate> for projects that need it, like server/build.gradle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't do this one yet, think I will leave it to another future PR
5c4f895 to
3c116a9
Compare
|
About the test-kit tests, I don't understand corretcly what should I do. Just a test to see if the status would be UP-TO-DATE on a repeated run? |
|
You can look at tests that implement @elasticmachine test this please |
|
@Megamiun you'll have to merge master to get the builds passing. |
|
Sorry for the delay, almost finished here. But I saw something, it seems this task can not be used as up-to-date. I put it to run with --info and received: It seems that by not existing output, gradle doesn't skip execution because of up-to-date status. Probably by tomorrow I can make the MR |
|
@Megamiun are you still interested in pursuing this PR ? |
|
Yes, thanks for remembering me, will try to do this weekend. But even so, @atorok, about the UP-TO-DATE, is it really the desired behaviour? |
Nope, we should fix this (although it doesn't have to be in this PR). The way we have done this in other cases is to use a "marker" file that we simply touch when the task completes, and configure that as the task output. For example, see ForbiddenPatternsTask. |
3c116a9 to
5b119c9
Compare
5b119c9 to
90add72
Compare
alpar-t
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elasticmachine test this please
buildSrc/src/main/java/org/elasticsearch/gradle/precommit/DependencyLicensesTask.java
Outdated
Show resolved
Hide resolved
| compile "com.carrotsearch.randomizedtesting:randomizedtesting-runner:${props.getProperty('randomizedrunner')}" | ||
|
|
||
|
|
||
| compile "commons-codec:commons-codec:${props.getProperty('commonscodec')}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we adding this would try to avoid increasing the transitive dependencies if possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this to add the possibility of encoding the sha1 byte array to an hex string.
It seems the way to do it is a little more laborious in pure java and the encodeHex method was a Groovy method.
If prefered, I can change the impl to use java directly.
And when making changes, I just need to rebase my branch, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, and the integTest task takes some hours(it didn't complete yet), on my computer, is it normal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Megamiun thanks for the explanation, I think that's good enough reason to add it.
Unfortunately our integ tests do take that long currently, there's many of them and we are working to optimize execution to make them faster.
@elasticmachine test this please
|
@elasticmachine tesdt this please |
|
@elasticmachine test this please |
|
@elasticmachine test this please |
|
Hello, tried to resolve the IT tests problem, is it possible to run again the tests? Unfortunately the time it takes to tuns the tests makes it not possible to test it on my pc |
|
@Megamiun you can run the test you added alone with this command: @elasticmachine test this please |
|
I think maybe there is a problem with my pc, it has been eleven hours and the integtests are still running. And I used the command to run only mine. I analysed the jarHell its and I tried to make some changes that to me seems to resolve the problem |
|
Hello, @atorok? |
| GradleRunner runner = getRunner("dependencies_missing"); | ||
|
|
||
| assertTaskFailed(runner.build(), ":dependencies_missing:dependencyLicenses"); | ||
| assertTaskFailed(runner.buildAndFail(), ":dependencies_missing:dependencyLicenses"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is needed, but when asserting for failure we should assert for something in the output to make sure we are getting the failure we want.
I can see that from the last test run we got Plugin with id 'elasticsearch.build' not found. so I don't think we are getting the intended failure here yet the test would pass.
The reason for the failure is that you must use the new Gradle syntax to apply the plugin. take a look at buildSrc/src/testKit/elasticsearch.build/build.gradle
|
Sorry, @atorok, I closed this PR by error. Now I am doing one more try of running the tests. I will open a new PR once I finish this. I found a way to run the integTests locally |
Relates to #34459