Import-Package and other requirements not translated as Maven #71

merks · 2022-07-11T14:12:42Z

dependencies when generating pom

Reduce the number of MavenMappings.

Use the new CBI aggregators features described here to map based Maven
metadata in p2 and to build Maven dependencies from java.package
requirements:

eclipse-cbi/p2repo-aggregator#11

Disable the validation repositories because they're not needed given the
platform repository itself contains all requirements, and keeping them
also results analysis anomalies because Orbit IUs are resolved where
direct-from-Maven IUs are actually in the repository.

#67

merks · 2022-07-11T14:23:20Z

Note that this requires the latest tools from https://download.eclipse.org/cbi/updates/p2-aggregator/tools/nightly/latest to try it in the IDE. Installing these will allow to edit both the resources in their structured editors:

I used the analyzer to ensure that every dependency maps to an artifact and that every version range resolves to an actual version that actually exists in Maven. The view gives an overview of all the generated poms. Here you can see how a package import is mapped to a dependency:

The log will contain information about potential problems or cases where a mapping is no longer needed.

mickaelistria

See question inline. Overall, I'm questioning whether the fact of adding more rules is a good smell.

mickaelistria · 2022-07-13T15:49:42Z

publish-to-maven-central/SDK4Mvn.aggr

-  <mavenMappings namePattern="org.w3c.dom.events" groupId="org.eclipse.orbit.bundles" artifactId="org.w3c.dom.events"/>
-  <mavenMappings namePattern="org.w3c.dom.smil" groupId="org.eclipse.orbit.bundles" artifactId="org.w3c.dom.smil"/>
-  <mavenMappings namePattern="org.w3c.dom.svg" groupId="org.eclipse.orbit.bundles" artifactId="org.w3c.dom.svg"/>
+  <mavenMappings namePattern=".*" groupId="\$maven-groupId\$" artifactId="\$maven-artifactId\$" versionPattern=".*" versionTemplate="\$maven-version\$"/>


I would rather not having such rule explicitly defined. Is there any chance someone needs/wants to set things differently? It seems to me we don't want to offer flexibility here.

I asked via email for your feedback in here but none was forthcoming:

eclipse-cbi/p2repo-aggregator#11

I think we/you can live with this explicit opt-in rule. I want to offer that flexibility to ensure that new behavior is opt-in rather than "we think this is best for you so you get it whether you want it or not."

"we think this is best for you so you get it whether you want it or not."

IMO, this usually results in better results. More configuration is more complexity, more room for errors; it is often less productive. I think this is the case here, and I don't see addition of this rule as an added-value over the current state (where this rule is hardcoded as default when possible) but more as an extra complication that doesn't make maintenance of this .aggr file, nor of the aggregator itself, easier.

Sometimes opinions differ. This option does not and will not require further/future maintenance.

The editor offers these pre-configured (correct) mappings to enable using p2 properties and to enable java,package requirement resolution:

mickaelistria · 2022-07-13T15:51:27Z

publish-to-maven-central/SDK4Mvn.aggr

+  <mavenDependencyMapping iuNamePattern="(?!.*(org\.eclipse\.)).*|org\.eclipse\.emf.*|org\.eclipse\.ecf.*|org\.eclipse\.jetty.*" namespacePattern=".*" namePattern=".*" groupId="!" artifactId="!"/>
+  <mavenDependencyMapping namespacePattern="java\.package" namePattern="(org.eclipse.jdt).internal.(compiler\.(apt|tool))" groupId="$1" artifactId="$1.$2"/>
+  <mavenDependencyMapping namespacePattern="java\.package" namePattern="(org.eclipse.jdt).internal.(compiler\.apt)\..*" groupId="$1" artifactId="$1.$2"/>
+  <mavenDependencyMapping namespacePattern="java\.package" namePattern="(org.eclipse.jdt)(.internal|.core)?.compiler.*" groupId="$1" artifactId="$1.core"/>
+  <mavenDependencyMapping namespacePattern="java\.package" namePattern=".*" groupId="*" artifactId="*" versionRangePattern="(.*)"/>


I don't fully get why those parts would be necessary over what's existing.The CBI aggregator resolves a requirement to a bundle and the mavenMappings resolve those into a GAV. Why is this extra type/configuration necessary?

This (mapping of package requirements and requirements in general) was described here:

eclipse-cbi/p2repo-aggregator#11 (comment)

But I already drew attention to that in the comment of this PR.

dependencies when generating pom Reduce the number of MavenMappings. Use the new CBI aggregator's features described here to map based Maven metadata in p2 and to build Maven dependencies from java.package requirements: eclipse-cbi/p2repo-aggregator#11 Disable the validation repositories because they're not needed given the platform repository itself contains all requirements, and keeping them also results analysis anomalies because Orbit IUs are resolved where direct-from-Maven IUs are actually in the repository.

merks · 2022-08-28T08:42:41Z

@akurtakov

This version I've checked with the aggregator analysis editor that all generated Maven dependencies resolve to ones that exist in Maven. The ones that don't are marked with ** and from the wrapped search you can see only the new never-before-published-to-maven plugin is marked that way:

This additionally generates dependencies based on package imports which seems important given how much bundle requirements are being replaced by package imports.

Publishing should of course use the latest version of the aggregator for the package import support (as well as for the new support to use the Maven coordinates from the p2 metadata):

https://download.eclipse.org/cbi/updates/p2-aggregator/products/nightly/latest

merks · 2022-08-29T11:32:30Z

@akurtakov Should I merge this now?

akurtakov · 2022-08-29T14:09:02Z

I would defer the decision to @mickaelistria or @sravanlakkimsetti as they spent time in this cycle to investigate publishing.

mickaelistria · 2022-08-29T15:42:23Z

While I have shared some concerns about the current approach compared to my "ideal" expectation, I still trust that @merks tested this well, and I reckon the current state is an improvement over current state. So I would be fine merging it if Ed is confident as well.

merks · 2022-08-31T15:29:25Z

@sravanlakkimsetti

I'd like to understand the maven publishing process better. Where can I learn more about how it works? The steps for EMF publishing to Maven are really quite complicated, and I only publish a small subset...

mickaelistria · 2022-08-31T15:37:26Z

There is https://wiki.eclipse.org/Platform-releng/Publish_To_Maven_Central which is more or less up-to-date. The effective entry point is /eclipse.platform.releng/publish-to-maven-central/CBIaggregator.sh , it does contain a bit of documentation inline about the process.

merks · 2022-09-01T10:04:15Z

@sravanlakkimsetti

One concern I have is with the pom enriching logic...

In particular I think this assumes that there is not already a name element.

eclipse.platform.releng/publish-to-maven-central/src/org/eclipse/platform/releng/maven/pom/ArtifactInfo.java

Lines 66 to 71 in b25099b

    
           public String toPomFragment() { 
        
           	try { 
        
           		fixData(); 
        
           		StringBuilder buf = new StringBuilder(); 
        
           		String indent = INDENT; 
        
           		element("name", indent, buf, this.name);

That method is called from here which assumes there is necessarily a description element:

eclipse.platform.releng/publish-to-maven-central/src/org/eclipse/platform/releng/maven/pom/EnrichPoms.java

Lines 76 to 82 in b25099b

    
           	if (!detailsInserted) { 
        
           		if (line.contains("</description>")) { 
        
           			out.append(info.toPomFragment()); 
        
           			detailsInserted = true; 
        
           		} 
        
           	} 
        
           }

I changed the logic to properly populate the name element and the description element, but plugins generally don't generally have a description so that element would be missing. I ran into this issue publishing EMF to Maven yesterday where the final Nexus check complained about the description being missing. I modified the EMF pom enhancer to set the description to the name if the description was missing, but now I've also restored the logic in the aggregator such that if there is no description the name will be used as the description, so the aggregator produces a pom like this:

This is what leads me to be concerned that the current enhancer logic will create two name elements.

Note too that org.eclipse.platform.releng.maven.pom.ArtifactInfo is full of assumptions about git.eclipse.org as the SCM.

I suppose it would be better to create a new issue...

mickaelistria · 2022-09-09T08:07:35Z

@merks can you please open a dedicated issue about the name/description concerns?

merks · 2022-09-09T08:16:21Z

@merks can you please open a dedicated issue about the name/description concerns?

I’m out of the office the rest of the day with only my iPhone. So not until much later.

merks requested a review from mickaelistria July 11, 2022 14:12

merks mentioned this pull request Jul 13, 2022

Remove explicit mappings for some Maven artifacts #72

Closed

mickaelistria reviewed Jul 13, 2022

View reviewed changes

merks force-pushed the issue-11 branch 2 times, most recently from e5222f8 to 0836891 Compare July 17, 2022 06:29

merks force-pushed the issue-11 branch from 0836891 to bd986ff Compare July 29, 2022 07:44

merks mentioned this pull request Aug 15, 2022

org.apache.jasper.glassfish and com.sun.el versions have gone backwards eclipse-platform/eclipse.platform.releng.aggregator#438

Closed

merks mentioned this pull request Aug 28, 2022

Validate all poms are resolvable #53

Closed

merks force-pushed the issue-11 branch from bd986ff to 340e029 Compare August 28, 2022 08:32

merks merged commit b25099b into eclipse-platform:master Aug 30, 2022

merks deleted the issue-11 branch August 30, 2022 06:00

merks mentioned this pull request Sep 9, 2022

Publish 4.25 to maven central eclipse-platform/eclipse.platform.releng.aggregator#548

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Import-Package and other requirements not translated as Maven #71

Import-Package and other requirements not translated as Maven #71

merks commented Jul 11, 2022

merks commented Jul 11, 2022

mickaelistria left a comment

mickaelistria Jul 13, 2022

merks Jul 13, 2022

mickaelistria Jul 14, 2022

merks Jul 14, 2022 •

edited

Loading

mickaelistria Jul 13, 2022

merks Jul 13, 2022

merks commented Aug 28, 2022

merks commented Aug 29, 2022

akurtakov commented Aug 29, 2022

mickaelistria commented Aug 29, 2022

merks commented Aug 31, 2022

mickaelistria commented Aug 31, 2022

merks commented Sep 1, 2022

mickaelistria commented Sep 9, 2022

merks commented Sep 9, 2022

Import-Package and other requirements not translated as Maven #71

Import-Package and other requirements not translated as Maven #71

Conversation

merks commented Jul 11, 2022

merks commented Jul 11, 2022

mickaelistria left a comment

Choose a reason for hiding this comment

mickaelistria Jul 13, 2022

Choose a reason for hiding this comment

merks Jul 13, 2022

Choose a reason for hiding this comment

mickaelistria Jul 14, 2022

Choose a reason for hiding this comment

merks Jul 14, 2022 • edited Loading

Choose a reason for hiding this comment

mickaelistria Jul 13, 2022

Choose a reason for hiding this comment

merks Jul 13, 2022

Choose a reason for hiding this comment

merks commented Aug 28, 2022

merks commented Aug 29, 2022

akurtakov commented Aug 29, 2022

mickaelistria commented Aug 29, 2022

merks commented Aug 31, 2022

mickaelistria commented Aug 31, 2022

merks commented Sep 1, 2022

mickaelistria commented Sep 9, 2022

merks commented Sep 9, 2022

merks Jul 14, 2022 •

edited

Loading