#443 - publish maven GAVC info into the P2 meta-data

[laeubi]

This publishes the info of the original artifact into the P2 metdata.

[mickaelistria]

I don't think this is enough to cover #443, the properties would be set, but the MavenDependencyInjector wouldn't consume them, would it?

[laeubi]

but the MavenDependencyInjector wouldn't consume them

If it currently does not do this it should do that. At least that are seems to be the way how it is published inside the repository. I don't have a test-case at hand, but at least it should then show up in a published P2-Update-site...

Now assume we consume from a P2 site an artifact that was published there with maven coordinates this should work as well?!

[laeubi]

I have put a breakpoint at org.eclipse.tycho.core.maven.MavenDependencyInjector.addDependency(ArtifactDescriptor, String) and can see that the IU has the neccesary properties at this point, lets see if we can made it available there....

[mickaelistria]

See #473 for a test case of what's expected here.

[laeubi]

@mickaelistria do you think we can merge this as a first valuable step? I could take a look at MavenDependencyInjector afterwards.

[github-actions]

Unit Test Results

143 files  143 suites   42m 28s ⏱️
225 tests 222 ✔️ 3 💤 0 ❌

Results for commit fba2b72.

♻️ This comment has been updated with latest results.

[mickaelistria]

I don't think it's worth merging it if there is no consumer for it.

[laeubi]

e.g the remap-artifacts-to-m2-repo mojo already consumes this information.

[mickaelistria]

e.g the remap-artifacts-to-m2-repo mojo already consumes this information.

It consumes it from artifact repository, and I believe the tweak you're adding here only affects the metadata repository. Artifacts are already holding information about Maven coordinates, I'm not sure (really not sure, not against) that adding Maven coordinates on installable units/metadata is a good idea as 1 unit can lead to different artifacts.

[laeubi]

Sorry for confusion, you are right. Anyways there is one IU per bundle artifact and tycho already stores this information there, just copied from an arbitrary site generated by tycho:

 <unit id='org.eclipse.equinox.device' version='1.1.100.v20210212-1143' singleton='false' generation='2'>
      <update id='org.eclipse.equinox.device' range='[0.0.0,1.1.100.v20210212-1143)' severity='0'/>
      <properties size='8'>
        <property name='df_LT.bundleVendor' value='Eclipse.org - Equinox'/>
        <property name='df_LT.bundleName' value='Device Access Service'/>
        <property name='org.eclipse.equinox.p2.name' value='%bundleName'/>
        <property name='org.eclipse.equinox.p2.provider' value='%bundleVendor'/>
        <property name='org.eclipse.equinox.p2.bundle.localization' value='plugin'/>
        <property name='maven-groupId' value='org.eclipse.equinox'/>
        <property name='maven-artifactId' value='org.eclipse.equinox.device'/>
        <property name='maven-version' value='1.1.100-SNAPSHOT'/>
      </properties>
      <provides size='6'>
        <provided namespace='org.eclipse.equinox.p2.iu' name='org.eclipse.equinox.device' version='1.1.100.v20210212-1143'/>
        <provided namespace='osgi.bundle' name='org.eclipse.equinox.device' version='1.1.100.v20210212-1143'/>
        <provided namespace='java.package' name='org.eclipse.equinox.device' version='0.0.0'/>
        <provided namespace='osgi.identity' name='org.eclipse.equinox.device' version='1.1.100.v20210212-1143'>
          <properties size='1'>
            <property name='type' value='osgi.bundle'/>
          </properties>
        </provided>
        <provided namespace='org.eclipse.equinox.p2.eclipse.type' name='bundle' version='1.0.0'/>
        <provided namespace='org.eclipse.equinox.p2.localization' name='df_LT' version='1.0.0'/>
      </provides>
      <requires size='6'>
        <required namespace='java.package' name='org.osgi.framework' range='1.2.0'/>
        <required namespace='java.package' name='org.osgi.service.device' range='[1.1.0,1.2.0)'/>
        <required namespace='java.package' name='org.osgi.service.log' range='1.2.0'/>
        <required namespace='java.package' name='org.osgi.util.tracker' range='0.0.0'/>
        <required namespace='java.package' name='org.eclipse.osgi.util' range='0.0.0'/>
        <requiredProperties namespace='osgi.ee' match='(&amp;(osgi.ee=JavaSE)(version=1.8))'>
          <description>
            org.eclipse.equinox.device
          </description>
        </requiredProperties>
      </requires>
      <artifacts size='1'>
        <artifact classifier='osgi.bundle' id='org.eclipse.equinox.device' version='1.1.100.v20210212-1143'/>
      </artifacts>
      <touchpoint id='org.eclipse.equinox.p2.osgi' version='1.0.0'/>
      <touchpointData size='1'>
        <instructions size='1'>
          <instruction key='manifest'>
            Bundle-SymbolicName: org.eclipse.equinox.device&#xA;Bundle-Version: 1.1.100.v20210212-1143
          </instruction>
        </instructions>
      </touchpointData>
    </unit>

or take a look at P2GeneratorImpl.getPublisherAdvice(IArtifactFacade, PublisherOptions) so this aproach is quite "standard" tycho and probably this information is later on copied to the artifacts.xml as well....

[laeubi]

@mickaelistria can we merge this given that obviously it is already the case in tycho for other parts to publish that information to the IU?

[mickaelistria]

we have no test nor use-case for it so far. So I'd rather adding code and behavior to maintain without value to balance it.

[laeubi]

org.eclipse.tycho.p2.publisher.P2GeneratorImplTest explicitly test this behavior for the generator so there is/was a use that was strong enough to put this into a central part of Tycho...

[laeubi]

Also see org.eclipse.tycho.p2.testutil.InstallableUnitMatchers.hasGAV(String, String, String, String)

[mickaelistria]

If the test is there and is already green, what does this change bring compared to current state?
If it allows to remove some other pieces of code, or performs better, solves some bug, or whatever substantial benefit, good; but here I just see code addition and am still struggling to find out what's the gain.

[laeubi]

good; but here I just see code addition and am still struggling to find out what's the gain.

It simply aligns with how Tycho behave filling a gap currently not discovered by any test. I'm just a bit curious your change about PGP was also added without any additional test or "gain" in tycho, so how is this different?
So following your argumentation it should haven't been added at all (it even introduce new public configuration properties without any documentation)...

[mickaelistria]

PGP support is a gain for Tycho because there is/was a concrete need from consumers to allow adding PGP signatures to artifact metadata. Although there is no test, there was a clear request and use-case to cover, and this was adding a clearly identified necessary feature for consumers.
Here I do not see what problem it does resolve, or what actual requirement does it fulfill, it seems more like the reason to do it is "because we can", but if it is so, it's not sufficient to balance the maintenance effort.
Please convince me this change is valuable and generate value for a real use-case and I'll happily merge it. What is the "gap"? What problem does it cause? What can't we do without this change that we'll now be able to do with it?...

[mickaelistria]

Can you please just merge this with #487 so they can be reviewed together? I'm loosing track and having both patches in the same PR would allow to highlight the value and facilitate review.

[laeubi]

What is the "gap"?

I don't know why it was added at the first place, but if other tycho components do add this information when they work with maven items its seem valid to do it here also. And it adds value to the story for #433 so we better can discover the origin of an artifact.

What problem does it cause?

Artifacts for maven target locations might be seen in the P2 metadata as if the where actually locally build units and not derived from an maven artifact (e.g. like we already do with pom=consider artifacts where this meta-data is already generated the same way as here)

What can't we do without this change that we'll now be able to do with it?...

I have already shown in #487 how it could be used to reliable recover maven GAV without fiddling around with the storage location of an artifact (e.g. if we consume one from an update-site that does not store the information on the right path so we can assume it is/was a maven artifact).

Can you please just merge this with #487 so they can be reviewed together?

But as this is not limited to Maven Target Location artifacts but any IU (e.g. even ones already published in P2 repositories) I think these are separate changes.