-
Notifications
You must be signed in to change notification settings - Fork 189
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
The target resolvement should never invalidate/error out, leave that up to the build. #400
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good start. See inline comments about how to make it less risky.
@@ -52,7 +52,16 @@ public ProjectorResolutionStrategy(MavenLogger logger) { | |||
|
|||
@Override | |||
protected Slicer newSlicer(IQueryable<IInstallableUnit> availableUnits, Map<String, String> properties) { | |||
return new Slicer(availableUnits, properties, false); | |||
return new Slicer(availableUnits, properties, false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the implementation of Slicer, there is a chance this adds into the resolution context some undesired units (ie units that were not present nor expected, and will now be present unexpectedly).
I believe the responsibility of "removing" not applicable requirement would belong to the caller here, ie the "availableUnits" should be filtered beforehand according to the context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so you are saying that the fix should be in AbstractSlicerResolutionStrategy.slice(Map<String, String> properties, IProgressMonitor monitor) ?
and then:
IQueryable slice = slicer.slice(seedIUs.toArray(EMPTY_IU_ARRAY), monitor);
we need to make sure that seedIUs don't have the stuff that can result in a false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that's the whole purpose of the thing, to filter out invalid items, if we let in items that are not applicable the whole "resolve for a target environment" does not makes any sense for me as it only adds overhead to the resolution time.
Instead I would instead use a different approach were we accept all units that satisfy any of the provided target environments...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i kind of agree with that, i think that whole resolvement is not really needed, as i said in the other discussion
For me a target file when installing (so how it is used in Tycho) is just a bunch of Installable units from the given locations.
For me nothing has to be really resolved, it must just supply everything that is needed and then the actual builds (plugins,features and product) will do the actual resolvement and reporting of errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the case and what we are seeing here is actually the part of targets called "Software Sites" where there are some strange requirements (possible due to the eclipse concept of singleton bundles) that only allows the "Planner mode" to resolve for exactly one environment: the checkbox: include all environments is grayed out if you select "include required software".
i also first thought about that one and i wanted to really overwrite validateInput: but that is private :( The thing is that ProjectorResolutionStrategy.java also used in scenario's where you would have that isApplicable() to be able to return false (and yes then something that we want does happen) |
It should later fail when the feature or plugin is actual build so we have more information
6b3cc6c
to
f5f5ae3
Compare
masters are open for 4.23 so you can open a gerrit review to make this protected :-) |
so i did a refactor now i just overwrite the slice() method and filter out first what would not be applicable (so validateInput() would bomb out) |
The |
As mentioned above |
But here, @jcompagner wants to use slicer, so it's fine. |
Nope, the example target uses planner mode:
|
BTW as tycho supports multiple targets, one could think of using a "regular" target file that includes everything except the offending feature and on with that feature in slicer mode and include both in the build. |
My bad! Should we try to support planner+includeAllPlatforms in Tycho, despite it's not in PDE? |
i have to say that i don't know what "planner" or includeAllPlatforms="false" or includeConfigurePhase="true" all really does But are you saying that if i break up the target and one have it alll (just as the current one with the same config as above) and 1 with just the chromium stuff but then includeAllPlatforms="true" and i add both targets in my platform that it then works? I just wonder are there scenario's when we really want an exception to be given from that validateInput() when resolving the target? When is the target really kind of illegal and we should really stop? |
I think this should only be done if its supported upstream (PDE+P2) otherwise it is too confusing and leads to bad user experience. As mentioned earlier I think the whole configuration here is a bit wrecked and making it "just work" has a bad smell. If one really likes to have such a behavior its better to fix the offending feature (and fork that for example) instead of putting the burden on the build tool. |
You are probably not alone ;-)
|
You can add multiple If it solves your problem depend a bit, I noticed that often projects don't mind to creating self-containing complete features what makes slicer mode a mess to use. |
How does PDE like this .target (or not)? Does it send an error, or the feature is silently included?... We could have Tycho mimicking that. |
Thanks @laeubi . So I don't think this should be changed in Tycho now; unless it's happening in a coordinated effort to have PDE also supporting this pattern without failing. |
yes that seems to work, and one target without the chromium urls (what our build system uses) But i can just when i update the full target with the latest urls and IU versions i can just copy that over the "without chromium" and remove only those urls from it. And then also update the pom file. because using has 1 annoying thing that you can't fix a version, and 1 product build should always results in the same artifacts, it can't be if a repository updated its thing that suddenly the product has different things. But happily chromium is provided with sub urls:
so i can still pinpoint to a version. I just need to maintain this pom file when i update chromium in the main target, which is fine by me. I do still think that whole target resolvement is to rigid it does stuff that it should not do at that time, its up to the system to decided on top of that if the target is ok or not. |
As noted above, it is not the target handling but the Update site location that is supposed to work that way, that interferes here with a too restrictive feature design of the Chromium Product... If you pay for that you should better ask them to fix their feature handling to be more adaptive... |
I think it has proven that there is no immediate need to change tycho here, so can we close this PR? |
i guess so, i have now a workaround that works ok for me It is for me still weird if i patch (this pull) it then i have "input" and i have my "output" and everything is working just fine.. But for some reason there is in the "input" a validation that just shouldn't happen, we can discuss that the input is kind of wrong, but that is for me besides the point, still i can get a valid output, noting wrong with the output i have.. But where is the problem then exactly? is it P2 or Tycho? |
Great!
Well its always possible to still have enough content in the target that it fulfills the requirement of a given setup. But the general contract of planner mode is that P2 has to calculate a satiable solution where all requirements are meet or else fail.
P2 target content has two modes:
for sure one might suggest a third mode called Available or something similar that includes as much as possible but never fails on missing requirements. That should then first be suggested to P2 then PDE, and if thats avaiable we can add support for Tycho as well. |
Directly PDE I think; p2 has nothing about Plugin Development in it, and p2 is already capable of consuming a whole repo as "available". |
PDE (and tycho) uses P2 under the hood to resolve those target locations, but I haven't checked yet if that's just a matter of using existing P2 features (this PR seems to indicate this) or if there are any changes required. Anyways I think this first should be implemented upstream. |
It should later fail when the feature or plugin is actual build
so we have more information (like stuff are optional)
see for a discussion (and a test case) here: #380