Lb task 1686 simplify domainmodel #1687
Conversation
|
Note that it might conflict with #1684 |
|
Can you also pr this here so that we know it works |
|
| <unit id="com.google.guava" version="30.1.0.v20210127-2300"/> | ||
| <unit id="io.github.classgraph" version="4.8.35.v20190528-1517"/> | ||
| <location includeAllPlatforms="false" includeConfigurePhase="false" includeMode="planner" includeSource="true" type="InstallableUnit"> | ||
| <!-- |
There was a problem hiding this comment.
an not sure (have no idea) how properly maintained our manifests are
and what would be fixed if we have
version x in manifest (no range)
and orbit has x and x+1
There was a problem hiding this comment.
if so should we also change the wizard?
There was a problem hiding this comment.
@cdietrich what do you mean by "change the wizard"? Do you mean the one that generates a new Xtext project? That was my next proposal ;)
If you have version x without no upper limit then it means that when the bundle will be installed it will use any latest version, so having a fixed version in the TP would probably be even worse, because you built it with a known version but it will be installed with a probably more up to date version.
Indeed, for objectweb.asm we have very strict version ranges in the MANIFEST, IIRC.
There was a problem hiding this comment.
unfortunately i really have no idea on target platforms and thus cannot judge if this is a good idea
There was a problem hiding this comment.
If you use "0.0.0" in the TP then the latest version will be used that respects the version ranges in features and bundles.
In any case, that's only for compiling. As I said above, if a feature or bundle is known NOT to work with a specific newer version, then a version range must be set explicitly in the feature or bundle: having a fixed version in the TP will only make sure you compile the code with that fixed version, but then users will still be able to install your feature or bundle with a newer (not working version) if you haven't specified the range correctly.
There was a problem hiding this comment.
i really would like a second or 3rd opinion here @kthoms @miklossy @ArneDeutsch
There was a problem hiding this comment.
No problem, let's wait :)
For sure, "slicer" should be avoided, but for the other parts let's see
There was a problem hiding this comment.
Looks like there won't be a second opinion coming soonish.
I can see why this makes sense for an example project but for "production" mode I'd rather pin the dependencies. Which raises the question if we want the examples to be different from what I personally would consider reliable practise for production environments.
There was a problem hiding this comment.
As you prefer. For sure, pinning dependency versions help make builds reproducible.
In any case, "slicer" makes no sense ;)
|
I'm pretty sure my proposal works, could any of the reviewers please take a look? @kthoms @miklossy @ArneDeutsch |
|
can you please rebase |
LorenzoBettini
force-pushed
the
lb_task_1686-Simplify_Domainmodel
branch
from
bd851c1 to
34970de
Compare
@cdietrich |
|
Has this been merged somewhere else? |
|
no |
|
and so? should I rebase it one more time? |
|
i still have doubts about non ranges in our plugins and new stuff show up in orbit and co. but i am not a tycho expert. |
|
feel free to close it |
|
I'm inclined to close this as there doesn't seem to be an agreement on how to do this. @LorenzoBettini can you help me understand the difference between slicer and planner? |
|
@szarnekow planner means that transitive dependencies are automatically downloaded, while slicer leaves transitive dependencies to your responsibility. Unless you specify a target platform for RAP, the only other reason to use slicer I can think of is masochism ;) |
|
Does this include optional transitive dependencies, too? |
As far as I know, yes. If I understand correctly, the "slicer" in the Domainmodel example comes from using the target platform DSL to create it (maybe @kthoms did that some time ago?) Another piece of information: PDE has always had a (big) limitation: if you want to have a target platform for all platforms (e.g., to create a product or application for Linux, Windows and macOS), you cannot use "planner" and must revert to "slicer" (that also might the be the cause of "slicer" in this example: it has been created with the Target editor by specifying "use all platforms"). If you don't need to build a product, there's no need to have includeAllPlatforms="true" (which, as I said, would force you to revert to "slicer"). Even if you need to build a product, nowadays, you don't use PDE wizards to export the products: you use Tycho, which handles the "all platforms" for you automatically and ignores the includeAllPlatforms="false" (and indeed, Tycho lets you specify a reduced set of possible platforms in its plugin configuration). So, there's really no point in using "slicer" (as I said, the only case is a target platform for RAP applications because you must have full control of what gets into the target platform: you can't have UI bundles inserted automatically in the target platform: you only need the RAP.UI bundles). |
|
i am not sure if the domain model examples is to test slicer as wizard uses planner |
|
So, I checked, and I remembered correctly: the change was introduced by @kthoms in 178c65e (using the target platform DSL, https://github.com/eclipse-cbi/targetplatform-dsl). However, I guess there has been some confusion with the overloaded term "platform". Testing with several target platforms has nothing to do with testing with several operating systems and architectures (platforms, but with another meaning). "includeAllPlatforms" means include all the specific stuff for all available OS and architecture (without that, you only have the native stuff for the current OS and architecture of Eclipse). It does not mean being able to test with several target platforms (e.g., with Oxygen, 2022-06, etc). This particular line in the target platform DSL 178c65e#diff-4c6daf77d2dfcd1860e3ee260366a29964189c8f93113ed9faffd4bbb8cec7b8R14 did the rest. The DSL then was no longer used, but the damage to the generated .target platform remained. I can't read @kthoms mind, but I guess that's the workflow that led to the wrong .target file. Well, not really "wrong": uselessly complex. If you don't trust me, please ask in the mailing lists or forums about the difference between planner and slicer. Again, when using Tycho, there's no need of slicer and includeAllPlatforms=true. If you don't want to create products, there's no need for "includeAllPlatform=true" (and so you can use planner). @mickaelistria once said he preferred to use "slicer" to control the target platform contents fully. Maybe he can confirm that or say that he changed his mind ;) Sorry to drag you in @mickaelistria, but I'm pretty sure you said that in some mailing list, which helped me understand the difference between slicer and planner. |
|
I still believe "slicer" is to be preferred whenever possible; it performs better and introduce less magic (aka "room for bugs") |
|
@cdietrich @szarnekow it's your call now. As I said, I'm 95% sure the use of slicer was not intentional |
Looking at other projects, e.g. lsp4e and wildwebdeveloper, planner seems favorable, though. No? |
|
planner is easier as you don't have to care about versions and so on; so it's cheaper to maintain. But slicer with finer-grain control would allow to achieve higher quality and better performance. |
|
Thank you for the input and the suggestions @LorenzoBettini @mickaelistria Since the domain-model example's target platform file is the only one in the entire Xtext codebase that uses slicer, I'm in favor of simplifying it to planner. @cdietrich would you be fine with it? |
| <location includeAllPlatforms="false" includeConfigurePhase="false" includeMode="planner" includeSource="true" type="InstallableUnit"> | ||
| <!-- | ||
| Just mention the p2 repo so that additional requirements with ranges specified in | ||
| features and bundles are taken from Orbit (e.g., objectweb.asm) |
There was a problem hiding this comment.
Do we want to stick to using the orbit update site?
There was a problem hiding this comment.
I guess it depends here: if we update to the next Eclipse version and the Orbit contents of the next Eclipse version are fine, then we should be safe.
Yes, for sure. I can also try to further simplify it (e.g., IIRC, PDE implies JDT) |
|
@cdietrich I see you approved. |
|
yes |
Closes #1686
I've tested the Maven build locally and it works also with tests