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

Add includeAllSources property to include all available source bundles #1120

Merged
merged 1 commit into from
Aug 24, 2022

Conversation

laeubi
Copy link
Member

@laeubi laeubi commented Jul 10, 2022

This is a first draft to fix #926 but currently misses an integration test to verify the functionality.

How it works:

  1. Let the slicer do its job
  2. Generate an IU that greedily fetches any source bundle using the usual naming scheme (@merks is there a more P2 way to find the source for a bundle than adding .source to the id?)
  3. Perform another slicer operation and adds the result to the final result.

@laeubi laeubi requested review from merks and HannesWell July 10, 2022 09:35
@github-actions
Copy link

github-actions bot commented Jul 10, 2022

Test Results

0 tests   0 ✔️  0s ⏱️
0 suites  0 💤
0 files    0

Results for commit f472406.

♻️ This comment has been updated with latest results.

@HannesWell
Copy link
Member

HannesWell commented Jul 10, 2022

Thank you @laeubi for working on this.
I can offer to work out an integration test, but I will be on vacation until Friday and likely won't have the time for that today.

Since I'm not familiar with the details I cannot assess them in detail but from what I understand it looks good

But I would like to clarify what sources are included exactly especially for different combinations of includeAllSources and includeAllDependencies.

With respect to includeAllDependencies the p2-repo created by Tycho has the following content:

  • includeAllDependencies=true
    -> All bundles/features listed in the category.xml are included plus all transitive requirements (bundles/features).
  • includeAllDependencies=false
    -> Only bundles/features listed in the category.xml are included and those bundles/features that are (transitively) included in the listed ones.

If one combines this with includeAllSources I would expect the following content:

  • includeAllDependencies=true and includeAllSources=true
    -> All transitive requirements (bundles/features) and the source of each feature/bundle in the repo is included.
  • includeAllDependencies=true and includeAllSources=false
    -> All transitive requirements (bundles/features) are included and sources are only included if they are included (transitively) in any feature listed in the category.xml.
  • includeAllDependencies=false and includeAllSources=true
    -> Only bundles/features listed in the category.xml are included and those bundles/features that are (transitively) included in the listed ones and the source of each feature/bundle in the repo is included.

So simply includeAllSources just ensures that for each bundle/feature in the repo, the corresponding source artifact is added as well (if not yet included anyways), regardless of how that bundle/feature made it into the repo.
Is that correct?

At the moment we have includeAllDependencies=true and includeAllSources=false for the Eclipse-Platform repo and could then switch to includeAllSources=true.

@laeubi
Copy link
Member Author

laeubi commented Jul 10, 2022

At the moment we have includeAllDependencies=true and includeAllSources=false for the Eclipse-Platform repo

I'm not sure if includeAllDependencies=true is used, but regardless of what is sued for every bundle contained in the final repository a source bundle will be included if found in the current target context.

@akurtakov
Copy link
Member

What is the status of this one ?

@laeubi
Copy link
Member Author

laeubi commented Jul 19, 2022

Code is there but @HannesWell wanted to provide a test-case for this so we can verify this works as intended.

@HannesWell
Copy link
Member

Code is there but @HannesWell wanted to provide a test-case for this so we can verify this works as intended.

Sorry I forgot a bit about that. I try to provide one before my (other) vacation next week.

@akurtakov
Copy link
Member

@laeubi would you please rebase it on top of master and guide whether and how I can publish extra commit to this PR with an integration test.

@laeubi
Copy link
Member Author

laeubi commented Aug 5, 2022

would you please rebase it on top of master

Will do this ...

and guide whether and how I can publish extra commit to this PR with an integration test.

Its actually quite simple, branch from current master, add an integration-test to demonstrate the issue and open a PR and we will see a failing test and thus proof a missing feature or regression. I can the cherry-pick that PR into my branch to proof my change fixes the test, so one could really work independently.

@laeubi laeubi marked this pull request as ready for review August 23, 2022 17:19
@laeubi
Copy link
Member Author

laeubi commented Aug 23, 2022

I plan to merge this once CI passes, we can enhance / add tests afterwards.

@laeubi laeubi merged commit 0e9acf0 into eclipse-tycho:master Aug 24, 2022
@laeubi laeubi added this to the 3.0 milestone Sep 21, 2022
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 this pull request may close these issues.

Enable inclusion of dependency-sources
3 participants