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

product / repository contains source jars by default #3522

Closed
sratz opened this issue Feb 20, 2024 · 24 comments · Fixed by #3635
Closed

product / repository contains source jars by default #3522

sratz opened this issue Feb 20, 2024 · 24 comments · Fixed by #3635

Comments

@sratz
Copy link
Contributor

sratz commented Feb 20, 2024

After migration from Tycho 4.0.5 to 4.0.6 I noticed that our product update site suddenly contains the source jar files even though none of the config was changed.

I bisected that to #3456, specifically https://github.com/eclipse-tycho/tycho/pull/3456/files#diff-9a9726aa0be8197a6082b700e8219a18531b5f9ba103b2f9fc5f162b154794eaR74

The related P2 change eclipse-equinox/p2#446 explicitly mentions that this new feature should be disabled by deafult.

Shouldn't

2053d37#diff-9a9726aa0be8197a6082b700e8219a18531b5f9ba103b2f9fc5f162b154794eaR74

set this to false?

@laeubi
Copy link
Member

laeubi commented Feb 20, 2024

@sratz source are resolved by default (if contained in the target) but the product / updatesite should only contain them if enabled, do you think you can provide a little example showing the problem?

@laeubi
Copy link
Member

laeubi commented Feb 20, 2024

By the way maybe it is because you include all requirements? The probably you want to disable source in the target altogether with targetDefinitionIncludeSource = ignore

@sratz
Copy link
Contributor Author

sratz commented Feb 20, 2024

do you think you can provide a little example showing the problem?

That would be some effort, I'll try.

By the way maybe it is because you include all requirements? The probably you want to disable source in the target altogether with targetDefinitionIncludeSource = ignore

The target file is shared among the maven reactor.

Based on this common target we produce several artifacts:

  • internal update site that does include the sources (explicitly by mentioning the source feature) for development/testing purposes
  • 'external' update site and products that does not include any sources.
  • ...

So I don't want to change anything in the target definition.

What about 2053d37#diff-9a9726aa0be8197a6082b700e8219a18531b5f9ba103b2f9fc5f162b154794eaR74

If I set this to false I get the old behavior, so I am wondering why this defaults to true here.

@laeubi
Copy link
Member

laeubi commented Feb 20, 2024

So I don't want to change anything in the target definition.

This is a configuration option in Tycho to override what is defined in the target

If I set this to false I get the old behavior, so I am wondering why this defaults to true here.

false will completely disable resolving of additional source bundles so they are not visible, as said what controls this is the one in the product builds, this only resolves possible items for Tycho to consider, so it all depends a bit on how you build your products / sites.

@laeubi
Copy link
Member

laeubi commented Feb 20, 2024

That would be some effort, I'll try.

@sratz I'll also try to take a closer look tomorrow but actually any tycho example that build a product from the integration tests should be able to reproduce the behavior if you for example just use the latest eclipse updatesite (that should contain the sources).

@sratz
Copy link
Contributor Author

sratz commented Feb 20, 2024

I got a reproducer:

pom.xml:

<?xml version="1.0" encoding="UTF-8"?>
<project xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd" xmlns="http://maven.apache.org/POM/4.0.0"
  xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
  <modelVersion>4.0.0</modelVersion>

  <groupId>tycho-its-project.product.selContainedWithoutSources</groupId>
  <artifactId>product.product</artifactId>
  <version>1.0.0-SNAPSHOT</version>
  <packaging>eclipse-repository</packaging>

  <properties>
    <!-- Important: This update site must have already been generated with the new P2 that adds the source as optional dependencyies with <filter>(org.eclipse.update.install.sources=true)</filter> metadata for the problem to show -->
    <target-platform>https://download.eclipse.org/eclipse/updates/I-builds/</target-platform>
  </properties>

  <repositories>
    <repository>
      <id>e342</id>
      <layout>p2</layout>
      <url>${target-platform}</url>
    </repository>
  </repositories>

  <build>
    <plugins>
      <plugin>
        <groupId>org.eclipse.tycho</groupId>
        <artifactId>tycho-maven-plugin</artifactId>
        <version>${tycho-version}</version>
        <extensions>true</extensions>
      </plugin>

      <plugin>
        <groupId>org.eclipse.tycho</groupId>
        <artifactId>target-platform-configuration</artifactId>
        <version>${tycho-version}</version>
      </plugin>

      <plugin>
        <groupId>org.eclipse.tycho</groupId>
        <artifactId>tycho-p2-repository-plugin</artifactId>
        <version>${tycho-version}</version>
        <configuration>
          <includeAllSources>false</includeAllSources>
          <includeAllDependencies>true</includeAllDependencies>
        </configuration>
      </plugin>

      <plugin>
        <groupId>org.eclipse.tycho</groupId>
        <artifactId>tycho-p2-director-plugin</artifactId>
        <version>${tycho-version}</version>
        <executions>
          <execution>
            <id>materialize-products</id>
            <goals>
              <goal>materialize-products</goal>
            </goals>
          </execution>
          <execution>
            <id>archive-products</id>
            <goals>
              <goal>archive-products</goal>
            </goals>
          </execution>
        </executions>
        <configuration>
          <products>
              <product>
                  <id>product.product</id>
              </product>
          </products>
        </configuration>
      </plugin>
    </plugins>
  </build>

</project>

product.product:

<?xml version="1.0" encoding="UTF-8"?>
<?pde version="3.5"?>

<product uid="product.product" id="product" application="application" version="1.0.0.qualifier" useFeatures="false" includeLaunchers="false">

   <plugins>
      <plugin id="org.eclipse.osgi"/>
   </plugins>

</product>

This results in

target/product.product-1.0.0-SNAPSHOT.zip:plugins/org.eclipse.osgi.source_3.19.0.v20240213-1246.jar
target/product.product-1.0.0-SNAPSHOT.zip:plugins/org.eclipse.osgi_3.19.0.v20240213-1246.jar

even though the source is not desired.

The materialized product on the other hand, does not have the source, only

target/products/product.product/linux/gtk/x86_64/plugins/org.eclipse.osgi_3.19.0.v20240213-1246.jar

@Phillipus
Copy link

Phillipus commented Feb 20, 2024

Same here with our product build using 4.0.6. Tycho is fetching source bundles into .m2/repository/p2/osgi/bundle and adding them to the product update site repository.

@laeubi
Copy link
Member

laeubi commented Feb 21, 2024

I'll take a look, but wanted to note (because it is often confused) that there is no such thing as "product update site", in this example you are building a repository that among others contains a product but this is usually not suitable as an update site for that product as it can contain many different things (depending on what is specified) and might lead to bad update behavior (e.g. always update the whole product / changes configurations / ...).

Instead one should have one product repository project and one site repository project (that contains a category.xml).

The product should then list all updatable items as root features, the category.xml should list them all and then you get a pure updatesite (e.g. one that do not contain your product definition).

@Phillipus
Copy link

I understand the distinction, but why does Tycho now fetch source bundles and add them to the product repository when it didn't do that in 4.0.5?

@sratz
Copy link
Contributor Author

sratz commented Feb 21, 2024

I think the problem is here:
When a product configuration is present, PublishProductMojo is active.
Its PublisherAction uses an ConfigCUsAction
https://github.com/eclipse-equinox/p2/blob/151d95ba4c27c2bc769bd6f3c0e55b11ff071d1f/bundles/org.eclipse.equinox.p2.publisher.eclipse/src/org/eclipse/equinox/p2/publisher/eclipse/ProductAction.java#L71
which in turn adds a tooling.source.default IU as dependency to the product, which causes all available sources to be included:
https://github.com/eclipse-equinox/p2/blob/151d95ba4c27c2bc769bd6f3c0e55b11ff071d1f/bundles/org.eclipse.equinox.p2.publisher.eclipse/src/org/eclipse/equinox/p2/publisher/eclipse/DefaultCUsAction.java#L53

And since eclipse-equinox/p2#446 those sources are now visible and are picked up.

@laeubi
Copy link
Member

laeubi commented Feb 21, 2024

@sratz nice finding, this is really nasty
@Phillipus is your product feature or bundle based?
@merks have you ever header about tooling.source.default IU ?

@Phillipus
Copy link

@Phillipus is your product feature or bundle based?

Feature based. Here.

@laeubi
Copy link
Member

laeubi commented Feb 21, 2024

@Phillipus can youprobabbly provide an integration-test to demonstrate the issue based on @sratz example and your product configuration (maybe stripped down a bit of course).

@Phillipus
Copy link

Phillipus commented Feb 21, 2024

@Phillipus can youprobabbly provide an integration-test to demonstrate the issue based on @sratz example and your product configuration (maybe stripped down a bit of course).

Sorry, I'm away from my dev machine for now.

@merks
Copy link
Contributor

merks commented Feb 21, 2024

There are always a whole bunch of "tooling" things whenever you build a product.

image

I can confirm that I see the same behavior for the product repository (or whatever you want to call it) having all kind of sources that are simply never used or needed.

Probably every product will have this problem so perhaps some existing integration test case also has this problem, but has no assertions to check for the problem?

@Phillipus
Copy link

Phillipus commented Feb 21, 2024

Here's a test consisting of pom.xml and test.product files. Unzip and run mvn clean verify to observe source bundles being downloaded into the .m2/repository/p2/osgi/bundle folder and added to the project's repository/plugins folder.

Please note - I am not a maven/tycho expert and know only just enough to do a build and run tests!

test.zip

@laeubi
Copy link
Member

laeubi commented Feb 21, 2024

Probably every product will have this problem so perhaps some existing integration test case also has this problem, but has no assertions to check for the problem?

Yes that's possible if I search for .product there are a lot of products used in the test but it seems no one ever noticed that behavior before maybe because if one wants to include sources previously always special action has to be taken but one never checked the other way round ... I have just no clue what this tooling.source is meant for maybe some kind of "make sources updatable"....

@Phillipus
Copy link

Here's a test consisting of pom.xml and test.product files.

Note that when setting includeAllDependencies to false source bundles are not included. However, our product needs that set to true.

@laeubi
Copy link
Member

laeubi commented Feb 21, 2024

@Phillipus thanks for testing and providing an example, the includeAllDependencies must maybe be tweaked here to ignore the new dependencies if includeAllSources is false (I assume you don't use that yet even though its good for OSS project to use that option).

@merks
Copy link
Contributor

merks commented Feb 21, 2024

I can't say the purpose of tooling.source.default but it provides a capability that products require:

image

It has an optional non-greedy (reluctant) requirement on all source bundles:

image

That are satisfied by each source bundle providing this capability:

image

It looks relatively pointless and harmless.

@merks
Copy link
Contributor

merks commented Feb 21, 2024

Even if I set includeAllSources to false, the repository still contains sources:

image

@laeubi
Copy link
Member

laeubi commented Feb 21, 2024

Even if I set includeAllSources to false, the repository still contains sources:

includeAllSources = false is the default, as said I think this needs some adjustments to now exclude things where previously it was only used to additionally include things.

@merks
Copy link
Contributor

merks commented Feb 21, 2024

Yes, looking at the code @sratz references, that context property was unconditionally added...

@laeubi
Copy link
Member

laeubi commented Feb 21, 2024

Yes, looking at the code @sratz references, that context property was unconditionally added...

As said this property is just that Tycho collects everything in the target (like it does for features) this does not mean it should be selected as one can see the product does not contain the sources ...

sratz added a commit to sratz/tycho that referenced this issue Mar 12, 2024
In the presence of products, P2 adds a virtual 'tooling.source.default'
IU which optionally depends on all sources.

If these sources are avaialble in the target platform, this would cause
them to be picked up even if assemble-repository was configured with
<includeAllSources>false</includeAllSources>.

This scenario has become much more likely with recent changes to P2 [1].

To prevent this, we actively prevent this from happening by ignoring the
requirements of the 'tooling.source.default' IU unless includeAllSources
is explicitly enabled.

These would be picked up even if If however, assemble-repository is

Fixes eclipse-tycho#3522.

[1] eclipse-equinox/p2#446
sratz added a commit to sratz/tycho that referenced this issue Mar 15, 2024
In the presence of products, P2 adds a virtual 'tooling.source.default'
IU which optionally depends on all sources.

If these sources are available in the target platform, this would cause
them to be picked up even if assemble-repository was configured with
<includeAllSources>false</includeAllSources>.

This scenario has become much more likely with recent changes to P2 [1].

We actively prevent this from happening by ignoring the requirements of
the 'tooling.source.default' IU, unless includeAllSources is explicitly
enabled.

Fixes eclipse-tycho#3522.

[1] eclipse-equinox/p2#446
sratz added a commit to sratz/tycho that referenced this issue Mar 18, 2024
In the presence of products, P2 adds a virtual 'tooling.source.default'
IU which optionally depends on all sources.

If these sources are available in the target platform, this would cause
them to be picked up even if assemble-repository was configured with

  <includeAllDependencies>true<includeAllDependencies>

but with

  <includeAllSources>false</includeAllSources>.

This scenario has become much more likely with recent changes to P2 [1].

We do not want includeAllDependencies to imply includeAllSources, so
we actively prevent this from happening by ignoring the

  <required namespace='org.eclipse.equinox.p2.eclipse.type'
            name='source'
            range='0.0.0'
            optional='true'
            multiple='true'
            greedy='false'/>

requirements if 'includeAllSources' is not specified.

Fixes eclipse-tycho#3522.

[1] eclipse-equinox/p2#446
laeubi pushed a commit that referenced this issue Mar 18, 2024
In the presence of products, P2 adds a virtual 'tooling.source.default'
IU which optionally depends on all sources.

If these sources are available in the target platform, this would cause
them to be picked up even if assemble-repository was configured with

  <includeAllDependencies>true<includeAllDependencies>

but with

  <includeAllSources>false</includeAllSources>.

This scenario has become much more likely with recent changes to P2 [1].

We do not want includeAllDependencies to imply includeAllSources, so
we actively prevent this from happening by ignoring the

  <required namespace='org.eclipse.equinox.p2.eclipse.type'
            name='source'
            range='0.0.0'
            optional='true'
            multiple='true'
            greedy='false'/>

requirements if 'includeAllSources' is not specified.

Fixes #3522.

[1] eclipse-equinox/p2#446
eclipse-tycho-bot pushed a commit that referenced this issue Mar 18, 2024
In the presence of products, P2 adds a virtual 'tooling.source.default'
IU which optionally depends on all sources.

If these sources are available in the target platform, this would cause
them to be picked up even if assemble-repository was configured with

  <includeAllDependencies>true<includeAllDependencies>

but with

  <includeAllSources>false</includeAllSources>.

This scenario has become much more likely with recent changes to P2 [1].

We do not want includeAllDependencies to imply includeAllSources, so
we actively prevent this from happening by ignoring the

  <required namespace='org.eclipse.equinox.p2.eclipse.type'
            name='source'
            range='0.0.0'
            optional='true'
            multiple='true'
            greedy='false'/>

requirements if 'includeAllSources' is not specified.

Fixes #3522.

[1] eclipse-equinox/p2#446

(cherry picked from commit 51e39d6)
eclipse-tycho-bot pushed a commit that referenced this issue Mar 18, 2024
In the presence of products, P2 adds a virtual 'tooling.source.default'
IU which optionally depends on all sources.

If these sources are available in the target platform, this would cause
them to be picked up even if assemble-repository was configured with

  <includeAllDependencies>true<includeAllDependencies>

but with

  <includeAllSources>false</includeAllSources>.

This scenario has become much more likely with recent changes to P2 [1].

We do not want includeAllDependencies to imply includeAllSources, so
we actively prevent this from happening by ignoring the

  <required namespace='org.eclipse.equinox.p2.eclipse.type'
            name='source'
            range='0.0.0'
            optional='true'
            multiple='true'
            greedy='false'/>

requirements if 'includeAllSources' is not specified.

Fixes #3522.

[1] eclipse-equinox/p2#446

(cherry picked from commit 51e39d6)
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 a pull request may close this issue.

4 participants