Skip to content
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

CXF-8700: Eliminate the java.compiler dependency from cxf-core #946

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

rotty3000
Copy link
Contributor

Signed-off-by: Raymond Augé [email protected]

@rotty3000 rotty3000 force-pushed the thin.cxf.core branch 2 times, most recently from b0f28d8 to 14ff3fd Compare May 7, 2022 20:58
@rotty3000 rotty3000 marked this pull request as draft May 7, 2022 21:01
@rotty3000 rotty3000 marked this pull request as ready for review May 8, 2022 02:05
@rotty3000 rotty3000 force-pushed the thin.cxf.core branch 2 times, most recently from 5656335 to f62cbd4 Compare May 11, 2022 00:04
@rotty3000
Copy link
Contributor Author

enabling Karaf debug settings in org.apache.cxf.systest.sts.itests.BasicSTSIntegrationTest didn't work :(

@reta
Copy link
Member

reta commented May 11, 2022

enabling Karaf debug settings in org.apache.cxf.systest.sts.itests.BasicSTSIntegrationTest didn't work :(

@rotty3000 could you please uncomment KarafDistributionOption.keepRuntimeFolder(), (BasicSTSIntegrationTest), I will fetch the distribution and logs right after the build.

@rotty3000
Copy link
Contributor Author

@reta done!

@reta
Copy link
Member

reta commented May 11, 2022

@reta done!

Could see a few things:

system.properties:

org.ops4j.pax.url.mvn.localRepository=/home/jenkins/workspace/CXF/CXF-JDK11-PR/.repository

/home/jenkins/workspace/CXF/CXF-JDK11-PR/.repository has cxf-tools-compiler published but the bundle is not installed:

karaf@root()> la | grep -i compiler
karaf@root()>

Looking why PAX does not seem to pick the artifacts.

@rotty3000
Copy link
Contributor Author

Oh, I see all checks have now passed :)

…er to lower footprint of `java.desktop` dependency

Signed-off-by: Raymond Augé <[email protected]>
…esent, reduces the footprint of the `java.desktop` dependency

Signed-off-by: Raymond Augé <[email protected]>
…dependency on `java.desktop`

Signed-off-by: Raymond Augé <[email protected]>
…ere it's used, reduce footprint of `java.desktop` dependency

Signed-off-by: Raymond Augé <[email protected]>
@reta
Copy link
Member

reta commented May 28, 2022

Thanks for the pull request @rotty3000! My apologies for the delay looking into it. May I ask you please to split this request into smaller ones. It seems like you have bundled java.desktop and java.compiler + other changes together but there are separate tickets for it (CXF-7000, CXF-7001 created by you). The compiler changes look good to me (besides the compatibility comments) but I do have questions to java.desktop related changes.

@@ -17,7 +17,7 @@
* under the License.
*/

package org.apache.cxf.common.util;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we please keep the same package? I understand that we would have package split brain but this is public API and if we want this change into 3.4.x / 3.5.x / 3.6.x, we should not break the possible consumers.

Alternatively, we could keep package change BUT introduce org.apache.cxf.common.util.Compiler class which will delegate to org.apache.cxf.tools.compiler.Compiler: it seems like public API is not using any dependencies from java.compiler APIs.

@@ -414,6 +414,11 @@
<artifactId>cxf-tools-common</artifactId>
<version>${project.version}</version>
</dependency>
<dependency>
Copy link
Member

@reta reta May 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we should add this dependency to cxf-core (which could be excluded on demand) to not break existing applications if anyone is using Compiler class (since this is public API).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants