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.

Sign up for GitHub

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

Jump to bottom

Add optional, greedy and disabled by default requirement to the source #446

Merged
merged 1 commit into from
Feb 4, 2024
Merged

Add optional, greedy and disabled by default requirement to the source #446

merged 1 commit into from
Feb 4, 2024

Conversation

laeubi
Copy link
Member

@laeubi laeubi commented Feb 4, 2024

Currently it is quite cumbersome effort to include the source of a bundle with an installation even though there might be multiple demands, for example legal reasons or debugging.

This now adds a new greedy, optional requirement that is disabled by a filter so it is not effective by default but enables an install agent to specify a property of org.eclipse.equinox.p2.includesource=true to include sources of bundles in products whenever available.

@laeubi laeubi requested a review from merks February 4, 2024 08:23
@laeubi laeubi mentioned this pull request Feb 4, 2024
Closed
merks
merks approved these changes Feb 4, 2024
View reviewed changes
Copy link
Contributor

@merks merks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very good and useful.

@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 4, 2024 •
edited
Loading

Test Results

    9 files  ±0      9 suites  ±0   32m 44s ⏱️ + 2m 14s
2 183 tests ±0  2 179 ✅ ±0   4 💤 ±0  0 ❌ ±0 
6 639 runs  ±0  6 628 ✅ ±0  11 💤 ±0  0 ❌ ±0 

Results for commit a6451e3. ± Comparison against base commit 7ced201.

♻️ This comment has been updated with latest results.

@laeubi laeubi mentioned this pull request Feb 4, 2024
Merged
@HannesWell
Copy link
Member

Sounds also reasonable for me as well.
Would it make sense to have a constant for the filter so that code enabling it can reference the constant?

@laeubi laeubi marked this pull request as ready for review February 4, 2024 14:32
@laeubi
Copy link
Member Author

laeubi commented Feb 4, 2024

I now tested this and in general it works with Tycho but just requires some additional configuration at the moment, I also take the opportunity to fix javadoc errors and extract some constants here, so now this should be ready to submit.

@laeubi
Add optional, greedy and disabled by default requirement to the source
a6451e3 
Currently it is quite cumbersome effort to include the source of a
bundle with an installation even though there might be multiple demands,
for example legal reasons or debugging.

This now adds a new greedy, optional requirement that is disabled by a
filter so it is not effective by default but enables an install agent to
specify a property of org.eclipse.update.install.source=true to
include sources of bundles in products whenever available.
@laeubi laeubi merged commit f169149 into eclipse-equinox:master Feb 4, 2024
9 checks passed
@sratz sratz mentioned this pull request Feb 20, 2024
Closed
sratz added a commit to sratz/tycho that referenced this pull request Mar 12, 2024
@sratz
assemble-repository: Prevent sources from being included inadvertently
380bcac 
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 sratz mentioned this pull request Mar 12, 2024
Merged
akurtakov added a commit to akurtakov/tycho that referenced this pull request Mar 14, 2024
@akurtakov
Adjust resolving tests for 4.31
4e118d2 
With eclipse-equinox/p2#446 and Tycho resolving
all possibly needed bundles source bundle ends up there in case it's
needed.
@akurtakov akurtakov mentioned this pull request Mar 14, 2024
Merged
akurtakov added a commit to eclipse-tycho/tycho that referenced this pull request Mar 14, 2024
@akurtakov
Adjust resolving tests for 4.31
15c00f1 
With eclipse-equinox/p2#446 and Tycho resolving
all possibly needed bundles source bundle ends up there in case it's
needed.
eclipse-tycho-bot pushed a commit to eclipse-tycho/tycho that referenced this pull request Mar 14, 2024
@akurtakov
Adjust resolving tests for 4.31
6fabbaf 
With eclipse-equinox/p2#446 and Tycho resolving
all possibly needed bundles source bundle ends up there in case it's
needed.

(cherry picked from commit 15c00f1)
akurtakov added a commit to eclipse-tycho/tycho that referenced this pull request Mar 14, 2024
@akurtakov
Adjust resolving tests for 4.31
10a46e5 
With eclipse-equinox/p2#446 and Tycho resolving
all possibly needed bundles source bundle ends up there in case it's
needed.

(cherry picked from commit 15c00f1)
sratz added a commit to sratz/tycho that referenced this pull request Mar 15, 2024
@sratz
assemble-repository: Prevent sources from being included inadvertently
bfffc6e 
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 pull request Mar 18, 2024
@sratz
assemble-repository: Prevent sources from being included inadvertently
a028cbf 
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 to eclipse-tycho/tycho that referenced this pull request Mar 18, 2024
@sratz @laeubi
assemble-repository: Prevent sources from being included inadvertently
51e39d6 
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 to eclipse-tycho/tycho that referenced this pull request Mar 18, 2024
@sratz
assemble-repository: Prevent sources from being included inadvertently
1cabdee 
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 to eclipse-tycho/tycho that referenced this pull request Mar 18, 2024
@sratz @eclipse-tycho-bot
assemble-repository: Prevent sources from being included inadvertently
3f6a704 
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
Reviewers

@merks merks merks approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@laeubi @HannesWell @merks