Encode in a feature requirement if it is a "require" requirement #138
Comments
|
@merks that's how far I can track this down, would be good if you can help sorting this out so we can have the "flag that everyone wants" in Tycho afterwards. |
|
I would not go down such a root. The IRequirement.getDescription() is just text and is generally not populated. It's not even mentioned here for populating via a p2.inf: https://wiki.eclipse.org/Equinox/p2/Customizing_Metadata#Capability_Advice Personally, I think to try to use information in the description to have semantic content/impact seems like a hack at best. I don't know why Eike brought up greedy/optional; that seems not relevant here other than as some analog to how includes are treated differently from requires when composing an update site from features... What was the behavior for Personally, I would just restore the original behavior however it was implement. Probably it was treating these two things as identical and no one every noticed or complained. Trying to add more metadata to IRequirement is definitely an ocean boiling exercise and will break 'bad' frameworks like the CBI p2 aggregator used for building the SimRel repository and Maven-published repositories. So my suggestion would be to look at the range, and if there is exactly one version that can satisfy the range, treat it as an include, otherwise treat it as an import. Yes, that's not perfect and would treat match="perfect" as an include. But given no one noticed and no one complained about the past behavior, I can't imagine that being a problem that needs to be solved with major effort. If you feel strongly that such information must be encoded to achieve perfection, I would encode additional information as IU properties instead. As a name for a flag that everyone wants, I'd suggest "includeAllFeatureRequirements" or perhaps "includeFeatureImports". Personally I think it would be better to be false by default, to avoid surprising but very impactful behavior changes (that 'nobody wants'). I wonder what @sratz thinks about this theme? As I mentioned, I think it's sufficient to restore the previous behavior via configuration and avoid significant further complications... (Note that I'll be traveling the next days and will not have so much time.) |
As described above, these produces identical metadata and thus such a plugin/feature will be included.
Its not about "perfection" it is about consistency. If we say that
So what happens is that you use a strict range and the Of course it will work the other way round if you don't like the inclusion at all, and likes to pin your version while before the stuff was not included now it suddenly is (and you will blame Tycho and getting angry).
I really would appreciate to not apply experience from one "domain" to all other "domains", obviously it was noticed and complained, otherwise no one would have bothered to mentioned it in the docs (and no one would have bother to try fixing this). And simply because one is not affected to not mean others are not affected as well.
I'm all open for that and most probably will provide a patch today, still the problem remains that such a flag is confusing if it do not do what is claims to and thus I'm aiming for a solution "at the root" so we can have a solution that really satisfies everyone and do not depend on surprising side-effects.
Could you probably propose a patch for the |
|
I know those things produce identical p2 data and I personally think it's fine to treat that identically in Tycho. In any case, as mentioned I do not have significant availability now until Monday. Let's see with @sratz thinks about this... |
No problem we do not need a solution right now, so better safe than sorry:
|
|
So I have been trying to better understand the core problem and tried try to summarize: What users typically want:
How Tycho works:
Why it typically worked in the way people expected it to:
So the result that 'everyone expects' is really just a convenient side effect of how the slicer works and how you model dependencies. And if one were to deviate from that "0.0.0" for @laeubi Your suggestion would be to explicitly remember the origin of a dependency in the p2 metadata, so that we can also explicitly make a decision later based on that flag when assembling an update site. I agree that explicit things are always better and that the previous behavior was indeed undefined. For Tycho 3.0.0 it sounds reasonable to make this explicit. That is:
|
Correct! Even "worse" the first case (
I think this would be most obvious also from the P2 meta-data side, because otherwise there is no real chance to recover this data reliable one might parse the feature again, but from P2 POV there is only convention, but not requirement that a given meta-data maps to a "real" feature. Ideally, given how e.g. currently there is a method |
|
Note that for the comment about With with regard to 0.0.0 being a constant source of confusion, one can argue it's 'just' bad tool support. Why can't this be detected and reported in the IDE? In fact, how often do builds failed because the MANIFEST.MF needed an increment but nothing in the IDE complained about it? But we really digress here. :-P (Oomph's version builder always detects when anything included by a feature has incremented its version and therefore the feature itself must increment its version, producing an error message, with a quick fix.) |
I'm not sure what you suggest here, but in PDE there are the following options (sadly only applied to either all or nothing):
while the first is What actually is "confusing" is, that this "synchronize at build" assumes that your are always build "all at once" (I assume actually it is assumed to be computed at the time of the build of the updatesite!) while with distributed artifacts (platform build, or any other where there is a split) this can become quite cumbersome because you need to touch the feature (so it gets a new version!) so changes in the plugin version is picked up.
You might want to contribute this to PDE/API tools then :-) |
|
This may be a stupid idea... I believe the only problem is that 'perfect' maps to a requirement that looks just like an include. What if we mapped 'perfect' x.y.z.q to [x.y.z.q,x.y.z.q_], i.e., to a range so tiny (note the appending of a _ to the qualifier or using that as the qualifier if there is none) that in actual practice, only a single artifact version would ever match. I know it's not perfect (pun intended), but it's expedient and I'm doubtful that anyone actually uses perfect (based on no facts whatsoever). As we've noticed, IRequirments can't carry additional metadata and to try to add that will be a long and painful exercise Representing the information as IU metadata (property) associated with the requirement can be done without API or serialization changes but is a bit non-trivial because requirements don't have any kind of identity; we'd need to specify a list of IU ID and version-range pairs. Or maybe we just keep a list of the includes and then we only need to specify ID and version pairs. It can certainly be done, but it just feels like overkill or a corner case... |
I think we should not replace one assumption by another :-) If we really think no one uses that, it might better be removed from the PDE UI, from the feature.xml and so on...
Maybe not arbitrary metadata, but adding a (fixed) attribute? Also given that
one might have more extensions here if it is considered to dangerous to extend the existing items?
Even though this might be considered even more complex, currently we have This of course will most likely require to duplicate the data partly to not break backward-compatibility, so a flag like
seems less disturbing ... |
|
I think it is important to note that this need for additional metadata is purely something needed by and used by Tycho. Even there, it's an implementation detail of how Tycho implements the interpretation of a category.xml. It's certainly not something that p2 itself needs or uses for resolving requirements. Arguably Tycho could implement this differently, so elevating this to p2 API is somewhat questionable... Looking more at implementation details, the current slicers and projector already use the concept of a 'strict version requirement' to determine if a requirement should be considered applicable. We see that here: We also see that filters are used for this purpose (applicability) as well:
That makes me think can't we reuse that approach? Included plugins and features can already specify a filter (indirectly via the os, ws, arch, nl properties) so marking those is not so suitable, but imported plugins and features cannot specify a filter in the feature.xml. So one could the the approach of creating a filter while publishing the feature.xml's imported plugins and features to p2 metadata. E.g., So the guard implementation in Tycho's slicer would be to check if it's a strict version range (if not it's excluded) and if so, check the filter whether it's explicitly marked/filtered to be excluded from slicing (catching the case of a perfect match import). For regular/normal p2 usage, the property would be false and the requirement would be respected as normal/before, even by older runtimes. Not only that, older runtimes could even read the filter and mirror it properly so there would be no compatibility problems. |
I think this is quite a bit to simple assumption:
And Tycho do that as well as it reuses P2 implementation... but as explained, "most users" don't want "strict" requirements it is always argued that
In the end, it doesn't matter much how exactly we do this, and if filters are a way to work that out I think this would be perfectly valid here of course!
Won't this work for any requirement (regardless of its strict or not?)
This sounds very interesting, do you think you can propose a patch for this in the |
|
I agree that Tycho is one of the most important consumers to consider given this is how most (practically all) projects produce their results! Yes, I will propose a patch for the FeaturesAction. With regard to In this case the mapping is from a *.product like this:
The problem is that the If we're not careful with the implementation details, this will end up included in the repository. Let me think about how we might kill two birds with one stone because in the ideal world, EPP would not need the p2.inf generator hack to solve its problem. Note though that in this case the opposite is needed, i.e., by default the requirement should be filtered out by normal p2 whereas with a feature.xml's include, by default the requirement should be filtered in. |
Maybe the same approach can be used here as well to encode this? I need to check this indetail but if I rember correctly Tycho uses an extension of ProductAction or similar to exactly overcome these limitations. |
|
Yes, I get the sense that a similar approach could be used and I'm pretty sure that @jonahgraham would be very happy to get rid of this "hack" in EPP's build... |
At laest for features of a category there seems to be |
|
I thought that property was used to make the feature.group depend on the feature.jar: In any case, I think something along these lines ought to be workable... I'm just swamped at the moment with all the details around getting the 2022-09 release out, but I will keep thinking about this and come up with something early in the next cycle. |
|
It seems Tycho ships an own MetadataAction here: that then creates a requirement like this: |
I could be wrong, but as fars as I know this is a marker if that feature should be shown to the user to be installable (in contrast to a feature that is just included somehow), e.g. if you mention it in a |
|
Looking at the code that processes imports, it seems an Here we see the editor doesn't complain about the
If we are to use the filter we'd need to handle the case of there already being a filter. We could use Note that org.eclipse.equinox.p2.publisher.eclipse.FeatureEntry.isRequires() does record the information we want... |
|
Looking at the code here: and the called method: it seems that there are already some code to handle expanding a filter.
Yes, that's true ... I think you can still iterate over the individual items from a filter, its just not very convenient last time I tried that but of course possible. Beside that, even if it is allowed, I never seen anyone using a filter here, so maybe it was just implemented for symmetric reasons and actually never used?
Yes that's the last place where this data is recorded. |
|
@merks any updates on this? |
|
No. |
|
I'm not sure when I can ever find time for this. 😞 Moreover, I'm a bit doubtful of the actual value of implementing this; given limited resource one has to question where to invest resource such that it has the most benefit to the most people. I noticed in particular the other week that the documentation here: explicitly states the following:
Changing the above described/documented Tycho behavior specifically for a feature to cover the one (in my opinion) dark corner case where an import specifies an exact version range and the assembled update site is desired not to have the bundle included seems (to me) like a needle-in-a-haystack case. I assume no consumer has ever reported this as a problem... In any case, I don't want to start a long back and forth discussion. The above comments are merely my opinions and the opinions of others may reasonably differ from mine. I'm just setting expectations about my outlook and my priorities. |
|
The documentation just describes the current limitation of P2 because user has complained, it does not describe a feature we specifically implemented. So what "most users" see simply differs on how you define this group of users. So the problem is not that one extra bundle gets in, but that features are suddenly transitively pulled in and that's more than just a bundle, because currently Tycho can only include all requirements (transitively) or only those with strict version ranges, but for those people that don't want to manually bump their feature just to get a changed dependency included, one need a way to include also version-ranged dependencies (that are actually also "direct" but not transitively. In any case it is not strictly required that this is implemented by you, it was just raised concerns about changing anything and then you brought up an alternative. But if you describe in what way you would be fine with a change, anyone else can implement it as well if it seems to suffice. |
|
Closing this as not being relevant anymore (for me) as it seems more profitable to get rid of outdated P2 technology in Tycho and moving on to more modern and flexible approaches: |
|
This issue is closely related to #424 and I think the same type of solution can be applied here relatively easily. |
|
I think this issue can be addressed in org.eclipse.equinox.internal.p2.publisher.eclipse.FeatureManifestParser.processImport like this:
I.e., add a filter |
that must be able to filter out (even though it named "requires" is is more a perquisite.... I have no idea why< it is named import... |
|
Yes, the above method is used to process imports (feature/plugin). So it's applicable exactly to the things you show above. |
Provide new FeatureEntry.createRequires methods that specify isImport, deprecating the older ones, and provide a field as well as accessors for isImport. Use that attribute in FeaturesAction.getFilter(FeatureEntry) to add a filter (!(org.eclipse.equinox.p2.exclude.import=true)) to the synthesized requirement. Update StringBuffer to StringBuilder in the otherwise-modified files. eclipse-equinox#138
Provide new FeatureEntry.createRequires methods that specify isImport, deprecating the older ones, and provide a field as well as accessors for isImport. Use that attribute in FeaturesAction.getFilter(FeatureEntry) to add a filter (!(org.eclipse.equinox.p2.exclude.import=true)) to the synthesized requirement. Update StringBuffer to StringBuilder in the otherwise-modified files. #138
|
The addition of a filter will allow a resolver to exclude the import requirements at its discretion. |
As discussed in eclipse-tycho/tycho#1281 in certain cases it is desirable to not include things that are marked as "require" in a feature into an update site.
Tycho is using a (customized) MirrorApplication for building a
Repositoryfrom the P2 metadata of a feature but encounters the following problem in theFeaturesAction:p2/bundles/org.eclipse.equinox.p2.publisher.eclipse/src/org/eclipse/equinox/p2/publisher/eclipse/FeaturesAction.java
Lines 248 to 252 in 4d9f31f
here every requirement is encoded as a requirement regardless of it is a
plugin,includes(=feature) orrequires(either bundle or plugin) of the form :so at the mirroring phase, Tycho has no way to distinguish if this is a something to consider or not because:
and
both create the same metadata that is:
Possible things to mitigate this:
requiresif theFeatureEntry#isRequiresoptional=falsebecause you cannot control forrequiresthat they are optional, so probably one can declare theFeatureEntry#isRequiresasnon greedyandoptionalas claimed here.The text was updated successfully, but these errors were encountered: