-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Migrate build system to Gradle #469
Conversation
Dependency with "manual" license configuration: dnsjava/dnsjava#63 |
Dependency with "manual" license configuration: oblac/jodd#717 |
Dependency with "manual" license configuration: bulenkov/Darcula#53 |
Dependency with "manual" license configuration: json-path/JsonPath#550 |
Dependency with "manual" license configuration: bobbylight/RSyntaxTextArea#299 |
Dependency with "manual" license configuration: x-stream/xstream#151 |
Dependency with "manual" license configuration: google/brotli#758 |
Dependency with "manual" license configuration: ben-manes/caffeine#325 |
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.
Hello @vlsi ,
First, thanks for this big piece of work, volume of work is pretty big !
I made a first review, I didn't expect migration from Ant to Gradle would lead to so much code to be frank.
As of me, for now I don't feel comfortable maintaining this new code but I guess it will come.
Anyway, maybe it would help a bit to create or update doc with something like:
- Old Ant feature => Location / classes in Gradle
Example: - ApacheJMeter_parent.xml => Location where it is generated
...
Thanks
* limitations under the License. | ||
* | ||
*/ | ||
|
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.
Just for maintenance and Gradle newbies, add a header comment explaining aim of the file ?
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 just thought "gradle buildsrc" Google search answers those kind of questions.
Do you still think it is worth clarifying?
buildSrc/subprojects/release/src/main/kotlin/org/apache/jmeter/buildtools/AutoClassifySpec.kt
Outdated
Show resolved
Hide resolved
buildSrc/subprojects/release/src/main/kotlin/org/apache/jmeter/buildtools/jgit/dsl/Git.kt
Outdated
Show resolved
Hide resolved
@pmouawad , have you seen https://github.com/apache/jmeter/blob/1a70d90e71b00d813ad0735ee9063c4f9e1f0808/gradle.md ? However, I do not think we really need a dictionary that maps "old Ant feature" to "Gradle feature". 99% of the times one would be fine with just trying |
a5ad14f
to
97e0d24
Compare
Codecov Report
@@ Coverage Diff @@
## master #469 +/- ##
=========================================
Coverage ? 53.53%
Complexity ? 9389
=========================================
Files ? 1008
Lines ? 61671
Branches ? 6913
=========================================
Hits ? 33013
Misses ? 26260
Partials ? 2398
Continue to review full report at Codecov.
|
734be13
to
1c9c7ab
Compare
Hi, I only get the follwing tasks shown after checking out your repo:
Build Setup tasks
|
@frschwab , you have to be using For instance:
|
I checked it out. I ran gw init and gw wrapper and gw eclipse. |
Why? |
I thought I could get rid of the error messages. But I get the exact same messages if I do not merge apache master branch into vlsi gradle - I just tried it out again. |
I have rebased the branch, moved Please retry. I don't use Eclipse, however I tried to import the project, and it works for me. |
Dependency with "manual" license configuration: eXparity/hamcrest-date#26 |
Dependency with "manual" license configuration: hamcrest/JavaHamcrest#264 |
3aa2763
to
46a9013
Compare
One final thing from my side: |
It is funny I was thinking behind exactly those lines, and I have already hidden a couple of tasks: It is kind of hard to tell what are those "main" tasks. Frankly speaking, I don't like Gradle's verbosity on generateMetadataFileForCorePublication / generatePomFileForCorePublication / publishCorePublicationToMavenLocal / publishCorePublicationToNexusRepository I would try to hide those, and replace with something like "generatePom".
Do you refer to batch test tasks? |
I can try to find some time in the next days.
I just found out that e.g.
Yes, I thought about the long list of batch tests in the first section. Also seems like these sections are getting ordered alphabetically, which is maybe not the same order in which someone would call these tasks. |
I've created gradle/gradle#10133 |
23ddb0a
to
4dd6da3
Compare
Dependency with "manual" license configuration: weisJ/darklaf#22 |
This is revival of #448
Note: below tricks might help to hide file movements:
A) GitHub UI: scroll the diff ("files changed") to the very end, and type the following to the browser console:
$$('div.empty').forEach(function(e) { e.parentElement.parentElement.style.display='none'; })
B) You can watch individual commits (e.g. open Commits ) and review individual ones
C) You can use
git log -M30% -l0 --stat -p -G.
on the command line to show non-trivial diffs only (hide movements)D) You can use
git diff -M30% -l0 --stat -p -G. origin/master
to see the full diffNote GitHub has a limit on the number of renamed files, so sometimes it shows a file as "deleted and inserted" even if that was a simple rename.
-l0
can be used to set "unlimited" number of "renamed files" ingit diff
/git log
.Then the diff shows
152 files changed, 5046 insertions(+), 10204 deletions(-)
.Note: license verification, IDE configuration, and release plugin is moved to https://github.com/vlsi/vlsi-release-plugins.
It reduces JMeter build scripts by ~1500 lines, and it improves build time (pre-built plugins are reused instead of building it from source).