tycho icon indicating copy to clipboard operation
tycho copied to clipboard

Enable inclusion of dependency-sources

Open HannesWell opened this issue 3 years ago • 12 comments

One reason to include dependencies into features is to make their sources available in a p2-repo build by Tycho (see e.g. https://github.com/eclipse-equinox/equinox.framework/pull/41#issuecomment-1103533152 and following comments). While all dependencies can be included automatically in a repository (using includeAllDependencies of tycho-p2-repository-plugin their sources are not included automatically.

If dependency sources would be included into a repo too (maybe optionally) and P2/PDE would also fetch them with the main-artifact, when the TP is set up to include sources (I have not tested it yet, maybe this already works), then there would be one strong argument less to include dependencies into features (IMHO no strong one would be left).

Although sources could be detected and fetched from Maven-Central auto-magically (as Mickael suggested: https://github.com/eclipse-equinox/equinox.framework/pull/41#issuecomment-1103634124) this would require permanent internet access which is not always the case (because one has poor connection or are for example behind a company firewall). Simply including the sources of dependencies into p2-repos too (on demand/by default), would hopefully prevent us from a lot of discussion and unhappy users/fellow-committers, while allowing us at the same time to enjoy the benefits of not including dependencies into features.

HannesWell avatar May 02 '22 21:05 HannesWell

One reason to include dependencies into features is to make their sources available in a p2-repo build by Tycho

I don't think so, you can simply include any source bundle/feature directly in the category.xml.

their sources are not included automatically

Sources are never included automatically you need to explicitly enable that

If dependency sources would be included into a repo too (maybe optionally) and P2/PDE would also fetch them with the main-artifact, when the TP is set up to include sources (I have not tested it yet, maybe this already works), then there would be one strong argument less to include dependencies into features (IMHO no strong one would be left).

As @mickaelistria noted elsewhere, why using then features at all? I really don't get this "don't include" thing we put a lot of effort in to having mostly empty features. If you don't care about the dependency why include the source? And if the dependency is important why not include it in the feature?

Simply including the sources of dependencies into p2-repos too (on demand/by default), would hopefully prevent us from a lot of discussion and unhappy users/fellow-committers,

What one should keep in mind is, that this easily blow up a repository considerably, and includeAllDependencies is not meant for development but for install time where users generally don't care at all about the sources.

Although sources could be detected and fetched from Maven-Central auto-magically (as Mickael suggested: eclipse-equinox/equinox.framework#41 (comment)) this would require permanent internet access which is not always the case (because one has poor connection or are for example behind a company firewall).

Well how they then develop e.g. maven plugins at all? And why should we put effort into updatesites (how do they get them without internet access?) for such special cases?

laeubi avatar May 03 '22 03:05 laeubi

@HannesWell @merks I'm currently investigating on this, can anyone provide an integration-test or example to demonstrate the issue in a smaller context?

laeubi avatar Jul 10 '22 06:07 laeubi

Does Tycho have an existing test for how generating a source feature with tycho-source-feature-plugin's source-feature goal works? It could be extended to add an additional bundle which is imported instead of included. Or the test could be copied and modified to use import instead of include.

merks avatar Jul 10 '22 07:07 merks

@merks We have https://github.com/eclipse/tycho/tree/master/tycho-its/projects/automaticSourceGeneration https://github.com/eclipse/tycho/blob/master/tycho-its/src/test/java/org/eclipse/tycho/test/sourceBundle/AutoGenerateSourceArtifactsTest.java

I think what @HannesWell has in wind is that the source must not be mentioned in any feature at all to be included in the update site, that would also cover transitive dependencies.

laeubi avatar Jul 10 '22 09:07 laeubi

I think what @HannesWell has in wind is that the source must not be mentioned in any feature at all to be included in the update site, that would also cover transitive dependencies.

Exactly. Basically it is about a switch to simply include the sources for every bundle and every feature that is included into the p2-repo, regardless of if that source artifact is included explicitly (either by including it into a feature or by mentioning it explicitly in the category.xml).

It should not depend on how a bundle/feature is included into the repo (mentioned explicitly or included transitively) or if it is from the reactor or the TP.

HannesWell avatar Jul 10 '22 11:07 HannesWell

Note that this is what I see now which is driving the generation/inclusion of sources:

image

merks avatar Jul 10 '22 11:07 merks

Exactly, this is because at the moment we have includeAllDependencies=true and includeAllSources=false for the Eclipse-Platform repo. But if we switch to includeAllSources=true that should not be necessary anymore.

I have described it more lengthy in https://github.com/eclipse/tycho/pull/1120#issuecomment-1179711047 how I think/expect this option to work. Basically for each bundle/feature in the repo, it is ensured that its source artifact is in the repo too (regardless of included 'explicitly' or not).

When the target-location is then set up with includeSource="true" it should fetch the sources if they are available, even if they are not explicitly (transitively) included in the listed IU. I have not yet verified that explicitly I expect that it is already happening.

HannesWell avatar Jul 10 '22 11:07 HannesWell

That would address the issue of what's in the p2 repository, which is great. I wonder though, by which mechanism is a source bundle installed and available in the Eclipse SDK itself? I expect that happens only by virtue of inclusion in a feature that is transitively included in the product. But that's an assumption...

merks avatar Jul 10 '22 12:07 merks

Maybe we are talking about different things, but why should I install the source itself? Or do you mean if I include the SDK in the target platform? Then PDE automatically search in the target for source bundles.

laeubi avatar Jul 10 '22 12:07 laeubi

I did already explain that yesterday:

https://github.com/eclipse-equinox/p2/pull/107#issuecomment-1179660741

The sources are in the SDK currently:

image

This is not a accident. It's because, as I mentioned yesterday and previously, people commonly use the running IDE as the target platform.

merks avatar Jul 10 '22 13:07 merks

Is the SDK build by Tycho or by Oomph or ...?

laeubi avatar Jul 10 '22 13:07 laeubi

I showed you a picture of the features that define the Eclipse SDK and specifically how they include source features. Wouldn't you expect that anything building something so defined, e.g., an Export Product action from within the IDE, would contain bundle sources given that's exactly what the definition specifies?

Then I showed a picture of the Eclipse project's Eclipse SDK download, which came from here:

https://download.eclipse.org/eclipse/downloads/drops4/S-4.25M1-202207061800/

I assumed the name of the zip file would be quite recognizable as one of the primary deliverables of each of the Platform project's quarterly releases...

Why would I show you, on a Tycho issue, something that's purely produced by Oomph. That would make no sense, even if Oomph were a build system, which it is not. Oomph can just be used to create installations exactly as they are defined by the product authors.

This picture should answer the question of how the Platform project builds its SDK:

image

merks avatar Jul 10 '22 17:07 merks