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

extraRequirements should be part of the tycho-surefire configuration instead of the target platform #43

Open
laeubi opened this issue Apr 10, 2021 · 12 comments

Comments

@laeubi
Copy link
Member

laeubi commented Apr 10, 2021

As mentioned in https://www.eclipse.org/tycho/sitedocs/tycho-surefire-plugin/test-mojo.html extra requirements are only meaningful to the tycho-surefire-plugin but are instead configured in the target-platform-configuration.

We should do the following:

  1. add support to configure this in the tycho-surefire-plugin instead
  2. add a big warning that this has changed and will be removed in the next release
  3. remove it from the target-platform-configuration
@laeubi
Copy link
Member Author

laeubi commented Apr 10, 2021

Strange enough the type in the code for an extraRequirement is org.apache.maven.model.Dependency but in fact this is not a maven-dep but only used as a container object to pass the necessary data...

laeubi referenced this issue in laeubi/tycho Apr 10, 2021
relates to #43

Signed-off-by: Christoph Läubrich <laeubi@laeubi-soft.de>
@mickaelistria
Copy link
Contributor

extraRequirements in target-platform-configuration can be useful for some other cases not related to tycho-surefire-plugin. It can be useful basically for anything that does requires to load or execute the bundle as it influence the target-platform and resolved dependencies, some other mojos can leverage it. It can also help a bunch to work around some deps management odditites. So in any case, we shouldn't remove this extraRequirements from target-platform-configuration.
tycho-surefire-plugin already has dependencies (https://www.eclipse.org/tycho/sitedocs/tycho-surefire-plugin/test-mojo.html#dependencies ) that is aimed at extending the dependencies. But it has some limitations, eg are those dependencies compatible with the target-platform and so on; but if we can remove these limitations, we could totally stop recommending using target-platform-configuration/dependency-resolution/extraRequirements from tycho-surefire-plugin and stick to dependencies.

@laeubi
Copy link
Member Author

laeubi commented Apr 11, 2021

it can be useful basically for anything that does requires to load or execute the bundle

I think we should provide explicit solutions for such use-cases, I think you are refereeing to something like eclipse-run mojo?

It can also help a bunch to work around some deps management odditites.

Are there any examples/bug reports? I think we should try to fix or provide better alternatives

But it has some limitations, eg are those dependencies compatible with the target-platform and so on; but if we can remove these limitations,

I'm currently working on a more generic solution for plugins to express extra requirements so they get passed to the P2 resolver. tycho-surefire can the use this as well I think.

@mickaelistria
Copy link
Contributor

I think we should provide explicit solutions for such use-cases, I think you are refereeing to something like eclipse-run mojo?

Yes, for instance. The point is that some mojos do not recompute a target-platform and use the one from the modules. So yes, we should probably move some mojos to resolve deps instead of reusing TP. But to me, it's not really something that's really going to provide a lot of value over current state.

Are there any examples/bug reports?

Basically every case where p2 has limitations (split packages, use constraints, attributes in general...) needs to use target-platfrom extraRequirements as workaround. And this has to be in the common TP and considered by all mojos, as these extraRequirements need to be present for compilation & execution.

I'm currently working on a more generic solution for plugins to express extra requirements so they get passed to the P2 resolver.

OK, can you please send a note the mailing-list about the proposal so we can discuss this.
Realistically, the target-platform-configuration/dependency-resolution/extraRequirements stuff will need to stay until p2 is fully OSGi capable (est. never); and as the issue it resolves are not happening with PDE (that does not use p2 for dep resolution but directly Equinox and the target-platform), we still need a Tycho-specific solution.
But additional.bundles in build.properties can be advertized as an alternative if it's similarly supported.

@laeubi
Copy link
Member Author

laeubi commented Apr 11, 2021

But additional.bundles in build.properties can be advertized as an alternative if it's similarly supported.

additional.bundles is actually not a real alternative but another provider for "extra-requirements"

as these extraRequirements need to be present for compilation & execution

The problem is, that compilation != execution and we are currently intermix things there. And then there is actually test compilation/execution. The current intermix (not only in tycho but also pde) makes it really hard to test things nicely compared to other solutions.

@laeubi
Copy link
Member Author

laeubi commented Apr 11, 2021

The Compiler Mojo also has extraClasspathElements while surefire has dependencies.

@mickaelistria
Copy link
Contributor

The problem is, that compilation != execution and we are currently intermix things there.

We can safely assume, a bit like Mavne itself, that execution is a super-set of compilation.
But anyway, I don't get the general idea: tycho-surefire-plugin has the dependencies. What makes that they are not good enough ? What's the user story that they cannot cover? Is it more an implementation limitation to get rid of, or a design issue?

@laeubi
Copy link
Member Author

laeubi commented Apr 12, 2021

The user-story is that I don't like to have different configuration in IDE and special configuration in tycho.

Because of that I'm currently try to add support for some 'missing pieces' and noticed that the current configuration seems to be different mainly because of some limitations with the current design.

@mickaelistria
Copy link
Contributor

The user-story is that I don't like to have different configuration in IDE and special configuration in tycho.

OK, but then I don't get what's bad with extraRequirements here and why moving them to tycho-surefire-plugin configuration would be any better as it would still be special configuration in Tycho?
I'm not too worried in adding support for X or Y that are IDE-standard; it's more the idea of dropping some features and moving some responsibilities from parts of Tycho to the other I'm more conservative about. Eg in this case, the idea is "just" to add dependencies to test execution. That IMO tells more that tycho-surefire-plugin has to make an extra resolution step to deal with its extra dependencies (wherever they come from), not that we need to fully reorganize the resolution process.

@laeubi
Copy link
Member Author

laeubi commented Apr 12, 2021

what's bad with extraRequirements here and why moving them to tycho-surefire-plugin configuration would be any better

extraRequirements are (if i don't configure one target per module) rather global, they are resolved and handled like first-class dependencies (e.g. import-package). and are used to resolve a (for me) vague 'its just required sometimes' instead of being specific, so it leaves me with a feeling to just adding something here and there and better never touch it again because it works but no-one knows why :-)

e.g. the documentation:

This parameter has only limited support for dependencies to artifacts within the reactor. Therefore it is recommended to specify extraRequirements
If there are implicitly required bundles (e.g. org.apache.felix.scr to make declarative services work)

leaves an inexperienced user in a rather pending state: Should he use it? When use it? Why SCR (the statement is in fact just true for PDE+Equinox in fact) does not work.... and so on.

So if (for whatever reason) I need additional things for a test (e.g. mock services, special connectors and configuration whatever) I would always expect to configure it at the surefire-mojo level, and not on some other mojos configuration and of course I probably don't want this one to drip into updatesites or compile-stages (as it currently is with extraRequirements).

I rather want to enhance the overall user-experience, make all this more flexible and simpler for the user, so one can simply start a new tycho project in minutes without reading many docs, search google for examples and gets frustrated rather than removing 'extraRequirements' specifically.

@mickaelistria
Copy link
Contributor

Should he use it? When use it? Why SCR (the statement is in fact just true for PDE+Equinox in fact) does not work.... and so on.

At least it's documented and we have many users using it without much complaint. So the current state seems simple enough. I sincerely don't think that getting rid of extraRequirements here is a very valuable goal. I'd be very happy to see dependencies working well, but if it involves some complex bi-directional side-effects between mojos, it may not be worth it overall.

make all this more flexible and simpler for the user,

Beware that flexibility and simplicity don't go hand-by-hand together. Flexibility can often bring complexity (both to implement and use). I'd rather see all focus on "simpler".

@LorenzoBettini
Copy link
Contributor

extraRequirements in target-platform-configuration can be useful for some other cases not related to tycho-surefire-plugin. It can be useful basically for anything that does requires to load or execute the bundle as it influence the target-platform and resolved dependencies, some other mojos can leverage it.

Indeed we are using that as one of the possible solutions for some problems related to a Maven plugin used for code generation when Eclipse dependencies and Maven dependencies are mixed (that concern JDT and its strange version ranges) to make sure some fragments are taken (eclipse/xtext-maven#146)

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

No branches or pull requests

3 participants