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

Allow to resolve with missing requirements and record those #468

Merged
merged 1 commit into from Jan 2, 2022

Conversation

laeubi
Copy link
Member

@laeubi laeubi commented Jan 2, 2022

Signed-off-by: Christoph Läubrich laeubi@laeubi-soft.de

@github-actions
Copy link

github-actions bot commented Jan 2, 2022

Unit Test Results

143 files  143 suites   35m 37s ⏱️
225 tests 222 ✔️ 3 💤 0

Results for commit 2edd657.

♻️ This comment has been updated with latest results.

…cord those

Signed-off-by: Christoph Läubrich <laeubi@laeubi-soft.de>
@laeubi laeubi merged commit 2edd657 into eclipse-tycho:master Jan 2, 2022
@mickaelistria
Copy link
Contributor

Please try to enrich commit message for such important change. This seems to have the capability to alter behavior quite noticeably and the commit message doesn't explicit the value provided by this change.

Comment on lines +41 to +43
void addMissingRequirement(IRequirement requirement);

Collection<IRequirement> getMissingRequirements();
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not appropriate as part of the ResolutionData interface IMO. ResolutionData is to configure the resolution and shouldn't incorporate information about resolution results as the ResolutionData can be reused across multiple builds. Please rework it.

Copy link
Member Author

Choose a reason for hiding this comment

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

ResolutionData can be reused across multiple builds

There are only 3 locations where it is used, in all those locations the ResolutionData is disposed after the method call.

@mickaelistria
Copy link
Contributor

You introduce some code here without any concrete consumer (I don't see anywhere in the code where setFailOnMissing(true) in invoked so this change doesn't bring value at the moment. It would be nice to just not merge such thing greedily without a more complete story of how this is supposed to be used and how it's supposed to serve end-users (eg with an integration test highlighting the value).

@laeubi
Copy link
Member Author

laeubi commented Jan 2, 2022

Please try to enrich commit message for such important change. This seems to have the capability to alter behavior quite noticeably and the commit message doesn't explicit the value provided by this change.

This does not alter any behavior without explicitly enabling it, so everything just works as before.

This is a prequisite for #470 and I just don't wanted to complicate this story more as it already is, thus I extracted some of the changes from that.

@mickaelistria
Copy link
Contributor

thus I extracted some of the changes from that.

My concern is more about merging such changes early without guarantee the whoke story works. IMO such change should be merged only when the "functional" patch that does create value is approved, otherwise the net impact is just that technical debt grows without increasing value.

@laeubi
Copy link
Member Author

laeubi commented Jan 2, 2022

thus I extracted some of the changes from that.

My concern is more about merging such changes early without guarantee the whoke story works.

I have a working example for this and tested it with some larger build-setups, anyways we already have some demands in the past for a similar feature so I'm very certain we can use it in one way or the other.

IMO such change should be merged only when the "functional" patch that does create value is approved, otherwise the net impact is just that technical debt grows without increasing value.

we can easily revert this change if necessary and as it does not require existing code to change its impact is very low.

@laeubi laeubi added this to the 2.7 milestone Jan 4, 2022
@laeubi laeubi mentioned this pull request Jan 7, 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.

None yet

2 participants