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

Check with m-enforcer for complete runtime classpath #164

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

Conversation

kwin
Copy link
Contributor

@kwin kwin commented Jul 1, 2022

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@kwin
Copy link
Contributor Author

kwin commented Jul 1, 2022

Currently the build fails with

[INFO] --- maven-enforcer-plugin:3.0.0:enforce (enforce-complete-plugin-classpath) @ aemanalyser-maven-plugin ---
[WARNING] Dependency org.osgi:org.osgi.service.configurator:jar:1.0.0 (provided) via org.apache.felix:org.apache.felix.cm.json:jar:1.0.6 not found as runtime dependency!
[WARNING] Dependency org.osgi:org.osgi.service.cm:jar:1.6.0 (provided) via org.apache.felix:org.apache.felix.configadmin:jar:1.9.24 not found as runtime dependency!
[WARNING] Dependency org.osgi:org.osgi.service.log:jar:1.3.0 (provided) via org.apache.felix:org.apache.felix.configadmin:jar:1.9.24 not found as runtime dependency!
[WARNING] Dependency org.osgi:org.osgi.service.coordinator:jar:1.0.2 (provided) via org.apache.felix:org.apache.felix.configadmin:jar:1.9.24 not found as runtime dependency!
[WARNING] Rule 0: org.apache.sling.maven.enforcer.RequireProvidedDependenciesInRuntimeClasspath failed with message:
Found 4 missing runtime dependencies. Look at the warnings emitted above for the details.

To me those look like valid missing dependencies, but I am not sure whether there is a code path which actually relies on those.

@kwin
Copy link
Contributor Author

kwin commented Aug 2, 2022

@bosschaert Any opinion/feedback on this PR?

@bosschaert
Copy link
Contributor

Hi @kwin what problem is this PR aiming to address?

@kwin
Copy link
Contributor Author

kwin commented Aug 2, 2022

#113 is one of the issue which would be solved, but also it checks that the referenced runtime dependencies (those which count during maven plugin execution) have the right version (i.e. the version the underlying dependencies were developed against). Further insights in https://github.com/apache/sling-maven-enforcer-rules#require-provided-dependencies-in-runtime-classpath-since-version-100.

@bosschaert
Copy link
Contributor

Seems useful @kwin !

One concern is that there is a big long list of excludes which might be tricky to maintain?

@kwin
Copy link
Contributor Author

kwin commented Aug 2, 2022

....which might be tricky to maintain?

IMHOn less maintenance effort than always manually going through all transitive provided dependencies to check if some other dependencies need to be made available at run time :-)

Also this PR fixes some actually wrong version which might easily break code at runtime if some downstream dependency relies on a method which has only been added in a newer version.

@bosschaert
Copy link
Contributor

Ok - sounds good. I once it's not marked as 'Draft' any more I'm happy to approve.

@kwin
Copy link
Contributor Author

kwin commented Aug 2, 2022

You would need to check the current warnings as outlined in #164 (comment).

@bosschaert
Copy link
Contributor

[WARNING] Dependency org.osgi:org.osgi.service.configurator:jar:1.0.0 (provided) via org.apache.felix:org.apache.felix.cm.json:jar:1.0.6 not found as runtime dependency!
[WARNING] Dependency org.osgi:org.osgi.service.cm:jar:1.6.0 (provided) via org.apache.felix:org.apache.felix.configadmin:jar:1.9.24 not found as runtime dependency!
[WARNING] Dependency org.osgi:org.osgi.service.log:jar:1.3.0 (provided) via org.apache.felix:org.apache.felix.configadmin:jar:1.9.24 not found as runtime dependency!
[WARNING] Dependency org.osgi:org.osgi.service.coordinator:jar:1.0.2 (provided) via org.apache.felix:org.apache.felix.configadmin:jar:1.9.24 not found as runtime dependency!

I don't think we need a runtime implementation of the configurator, configadmin, OSGi log service, OSGi Coordinator for the maven plugin.

@kwin kwin marked this pull request as ready for review August 3, 2022 12:13
@cziegeler
Copy link
Contributor

I'm also worried by the long exclude list, in addition, the dependencies that are mentioned are not needed. So the marking of dependencies seems to be wrong.
This change is also updating/changing dependencies. At this point these are aligned mostly with what is used in Apache Sling's CP2F converter.

@kwin
Copy link
Contributor Author

kwin commented Aug 26, 2022

Well, the issues #176 and #173 show that maintaining the dependencies manually doesn’t really work

@cziegeler
Copy link
Contributor

cziegeler commented Aug 29, 2022

There is no flawless approach, look at the issues we had with the here suggested approach in Apache Sling, for example the feature launcher. In the end we are exchanging one imperfect way with another. It requires automated testing to verify that everything is still working

@kwin
Copy link
Contributor Author

kwin commented Aug 29, 2022

The approach of manually maintaining all necessary artifacts does not change at all with this PR. Also more automated tests definitely help. The only thing which is proposed here is a Maven plugin giving some hints on how to make the class path more complete (granted, with false positives but compared to no help at all this is imho still an advantage)

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.

3 participants