-
Notifications
You must be signed in to change notification settings - Fork 53
[BUILD][E] Create update-site via gradle #1030
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
stefaus
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.
LGTM
buildSrc/src/main/java/saros/gradle/eclipse/CategoryPublisher.java
Outdated
Show resolved
Hide resolved
eclipse/build.gradle.kts
Outdated
| from("feature/feature.xml") { | ||
| into("features") | ||
| } | ||
| into("plugins") { | ||
| from(project.tasks.findByName("jar")) | ||
| from(project(":saros.core").tasks.findByName("jar")) | ||
| } | ||
| from("update/site.xml") | ||
| into(updateSiteDirPath) |
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.
Unify usage of into...from.. logic? This uses 3 layouts. from->into with braces, into->from with braces, and from->into without braces.
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 I could change the first closure into a into{from} (removing one layout).
However, the remaining two layouts are not interchangeable:
- The "plain" layout at the bottom of the task (l.103-104) defines the root target dir (
into) and what should be copied into the root dir (from) - The previous from/into-definitions define target dirs relative to the root dir.
eclipse/build.gradle.kts
Outdated
| dependsOn(":saros.core:jar", ":saros.eclipse:jar") | ||
|
|
||
| from("feature/feature.xml") { | ||
| into("features") |
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 does this work? I.e., why can you just use the path relative to build/update-site here while you still seem to have to provide the base path for the site.xml (l.103).
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.
The last into defines the base path.
I think I should change the order and place the root into at the top of the specification.
See here for a detailled explanation.
buildSrc/src/main/java/saros/gradle/eclipse/CategoryPublisher.java
Outdated
Show resolved
Hide resolved
eclipse/build.gradle.kts
Outdated
| val updateSiteCategoryPublishing by registering { | ||
| dependsOn(updateSiteFeaturesAndBundlesPublishing) | ||
| doLast { | ||
| with(saros.gradle.eclipse.CategoryPublisher("4.8.0")) { |
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.
Is this the specification of the targeted Eclipse release? If so, is there a way of unifying it with the declaration in SarosEclipseExtension so that we don't have to specify the version in two different places?
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.
👍 Sure
eclipse/build.gradle.kts
Outdated
| register("dropin", Zip::class) { | ||
| dependsOn(updateSite) | ||
|
|
||
| archiveFileName.set("saros-dropin.zip") |
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.
Specify that it is Saros/E and the contained version?
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.
Do you mean something like saros-eclipse-dropin-<version>.zip? Sure
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 started to implement this but realized that I have to implement a proper version handling (as we need it for #808). This task is only executed to create a release and the current process is also to rename the zip manually. Therefore, I would rename the archive to "saros-eclipse-dropin.zip" without the version nr.
| destinationDirectory.set(project.file("build/dropin")) | ||
|
|
||
| from(updateSiteDirPath) { | ||
| exclude("*.jar") |
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 does this exclude the jar files?
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.
It excludes the content.jar and artifacts.jar as described here
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 would have expected that the cleanup was already done in l.167 - l.176.
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.
The cleanup you mention removes all metadata that are not part of the update-site, but these metadata are part of the update-site, but not part of the dropin.
ddbddf6 to
b4c0c76
Compare
|
I will rebase and introduce requested changes as soon as my PR in goomph is merged & released (allows to remove the custom |
We are currently using a workaround that allows us to import gradle into eclipse (via the prepareEclipse task). However, it would be possible to modify our build and import the project via the default eclipse gradle importer (buildship). This import does not detect projects as "features" and "update" which are not managed by gradle. This would lead to the situation that we are not able to create release artifacts. With the following gradle tasks we can create an eclipse update-site or dropin using the recommended approach via the P2Director.
cbe1fe6 to
d01031e
Compare
d01031e to
01b2b47
Compare
|
@tobous Are there still open change requests? |
|
Complexity increasing per file
==============================
- buildSrc/src/main/java/saros/gradle/eclipse/SarosEclipsePlugin.java 1
See the complete overview on Codacy |
We are currently using a workaround that allows us to import gradle into eclipse
(via the prepareEclipse task). However, it would be possible to modify our build
and import the project via the default eclipse gradle importer (buildship).
This import does not detect projects as "features" and "update" which are not
managed by gradle. This would lead to the situation that we are not able to
create release artifacts.
With the following gradle tasks we can create an eclipse update-site or
dropin using the recommended approach via the P2Director.