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

[bnd-run-maven-plugin][bnd-testing-plugin] specifiy bndrun file in the pom.xml directly #5496

Closed
laeubi opened this issue Jan 8, 2023 · 18 comments
Labels
abeyance need of contributor [requires is:closed]

Comments

@laeubi
Copy link
Contributor

laeubi commented Jan 8, 2023

Currently bnd-run-maven-plugin requires a bndrun file as a file.
bnd-maven-plugin allows to inline a bnd file as CDATA.

it would be good to have such an option for bndrun as well.

@laeubi
Copy link
Contributor Author

laeubi commented Jan 8, 2023

The same applies to bnd-testing-maven-plugin

@timothyjward
Copy link
Contributor

The big issue with this is that bndrun files aren’t just build configuration, they also have content that is usually generated (the runbundles). Maven plugins should not (and possibly cannot) modify the pom file, making inline bndrun content either fairly useless, or having to be merged with a separate file anyway.

@laeubi
Copy link
Contributor Author

laeubi commented Jan 8, 2023

The pom-file cannot be modified but the pom (object) can. Anyways it is bad to rely in a build on modifying an exiting (repository managed) files, so if there is a need for a previous stage to pass data it should:

  • use a file generated into target folder that can be used
  • use some kind of session variable and / or component to exchange
  • compute the value on demand if missing
  • ...

So in case you described the bnd-resolve (?) plugin could write a file target/resolved-bnd.bundles and then bnd-run-plugin can read that file in case it does not find the information in the bndrun file, but the runfile itself should not be required for such data-passing concerns.

So if you say a static bndrun would be useless, this actually shows a design-flaw here than a requirement as even for a file I would assume this to being "static" while the build runs.

@bjhargrave
Copy link
Member

Using inline bndrun files was discussed and rejected in #4476. The same issues still apply. A bndrun file is input to multiple bnd maven plugins (resolve, testing, run, export). So inlining a bndrun file would require duplication of configuration since maven does not really provide a way for multiple plugins to share some common POM configuration. The resolve plugin writes an output bndrun which can then be input to testing, run, export.

So I don't think it is a useful change to support inline bndrun files for the bnd maven plugins which process bndrun files.

@laeubi
Copy link
Contributor Author

laeubi commented Jan 8, 2023

A bndrun file is input to multiple bnd maven plugins (resolve, testing, run, export).

It can be but why should it be required to use all of these?

So inlining a bndrun file would require duplication of configuration since maven does not really provide a way for multiple plugins to share some common POM configuration

Why not let the user decide if it is acceptable to "duplicate" things? I think it won't be much of an effort to allow defining it inline so users have a choice.

Even the documentation says that a lot of things are inherited from the maven config already, but at the moment the execution is just skipped if there is no bndrun file so one needs to at least provide an empty one so the plugin actually do anything even if one wants it to inherit everything from the pom...

@timothyjward
Copy link
Contributor

Anyways it is bad to rely in a build on modifying an exiting (repository managed) files

So if you say a static bndrun would be useless, this actually shows a design-flaw here than a requirement as even for a file I would assume this to being "static" while the build runs.

So would you say that this model is also broken for the npm package-lock.json? That file is generated by a build command, checked into source control and used in CI. This is the same as the typical usage of a bndrun file.

Either manually or as part of a local build a developer can calculate the run bundles, check that list in, and then get failures in CI if the list is no longer correct.

@laeubi
Copy link
Contributor Author

laeubi commented Jan 8, 2023

So would you say that this model is also broken for the npm package-lock.json? That file is generated by a build command, checked into source control and used in CI. This is the same as the typical usage of a bndrun file.

As long as the build does not modify the file it does not really matter how it is generated, but a file should either be generated by the build (and not checked in) or stay the same regardless of how often I run the build.

Either manually or as part of a local build a developer can calculate the run bundles, check that list in

And why should a developer not being allowed to put that file-content into the pom.xml then? :-)

So if I have computed/created/... the bndrun-file it should be static, and if it is my choice I would like to put this content into the pom.xml ... if that means additional steps if there are changes, this is not the concern of the maven-plugin developer. One reason might be that I have crafted everything carefully, have checked every single license of each bundle and actually want the CI fail if anything changes here, so an automated approach won't reveal that problem.
Then I can use whatever tool to generate the updated run file, again carefully review everything and then copy it to the pom...

@timothyjward
Copy link
Contributor

a file should either be generated by the build (and not checked in) or stay the same regardless of how often I run the build.

This simply isn’t the case. Some files are too hard to write accurately by hand (excluding trivial cases) but can be generated based on analysis by a tool. These files should be generated. If the result of that needs to be used in CI to validate something then you should check it in. This is exactly the package-lock.json/bndrun use case.

So if I have computed/created/... the bndrun-file it should be static, and if it is my choice I would like to put this content into the pom.xml ... if that means additional steps if there are changes, this is not the concern of the maven-plugin developer.

As one of the authors of the bnd-resolver-Maven-plugin I can tell you that it is my concern. That plugin’s job is to calculate the run bundles and add them to the bndrun to save a developer from manual steps.

One reason might be that I have crafted everything carefully, have checked every single license of each bundle and actually want the CI fail if anything changes here, so an automated approach won't reveal that problem.

This is exactly the scenario I used with in my second comment. CI builds do a resolve, compare the result with the run bundles in the bndrun (a checked in file) and fail the build if there are differences.

Then I can use whatever tool to generate the updated run file, again carefully review everything and then copy it to the pom...

This would be the bnd-resolver-maven-plugin. So you would like to add another option with worse usability than the current case where the plugin can update the file for you, and that file can be
used in the testing/run/export plugins rather than copy/pasting large chunks of frequently changing generated configuration…

@laeubi
Copy link
Contributor Author

laeubi commented Jan 9, 2023

Nothing you wrote is wrong, still there is no reason why such generated data must be updated at build time! I can run these tools on my dev maschine and then put the result into my pom. And given you have a smaller library these don't frequently change nor are large chunks.

Just use this example from this repository:
https://github.com/bndtools/bnd/blob/master/demo/old-launcher.bndrun

and if I use the inferred vales from here
https://github.com/bndtools/bnd/tree/master/maven-plugins/bnd-run-maven-plugin#bndrun-details-inferred-from-maven

it even becomes shorter, so there not much left. So the only thing left is now the mentioned run-bundles, I might let me generate that list, but I even might want to maintain it by hand (for whatever demands). An at least @kwin has had such a requirement as well in #4476 so it seems there are (maybe rare) cases where it is useful, but supporting this is nothing complex (you can even write it out to a temp file in target if that makes coding easier).

@laeubi
Copy link
Contributor Author

laeubi commented Jan 10, 2023

This would be the bnd-resolver-maven-plugin.

I just noticed that the maven-testing-plugin even has parameter:

resolve Whether to resolve the -runbundles required for a valid runtime. Defaults to false.

So I would understand it that in such a case I won't need the bnd-resolver-maven-plugin to modify my bndrun file if setting this to true...

@bjhargrave
Copy link
Member

Correct. But you may also have a run and export plugin configured to use the same bndrun file(s), so having a discrete resolve step is then useful.

See also #4464 where it was discussed to remove the resolve configuration option from the testing (and other) plugin.

@laeubi
Copy link
Contributor Author

laeubi commented Jan 10, 2023

But that's the point (and I agree with @njbartlett here) in some setups it might be useful to split this in different phases ad do all kind of sophisticated stuff, but still there are use-cases where just simple invocations are required, so why not support both use-cases? Having split all these mojos over different maven plugins is already quite inconvenient but spreading required tasks over different plugins and goals makes it then really hard to use and one needs a lot of domain-specific (bnd) knowledge and documentation.

Just compare this with the maven counterpart https://maven.apache.org/surefire/maven-surefire-plugin/usage.html:

  • add a mojo execution to your pom
  • write a test
  • works with zero configuration (most of the time...)

So if one want to encourage users to use bnd more, it should work similar easy with just using good defaults, in case of the bnd-testing plugin that would be:

  • add a mojo execution to your pom
  • write a test
  • works with zero configuration (most of the time...) by:
    • if current module is not a bundle automatically add whatever is required by wrapping the testclasses+impl in a test-artifact with automatic derived headers including Test-Classes with the usual surefire-searchpattern **/*Test.class
    • use maven dependencies
    • select any framework (e.g. felix or equinox whatever seems "best") or just scan the classpath for FrameworkProvider SPI
    • select the current bundle as root and resolve

So by default no bnd files no bndrun files, no adding this and that plugin and wire them together... for sure the current approach "works" but it requires a lot of special knowledge and experience with bnd, bnd workflow, OSGi and so on so making it more and more general also mean it is loosing value of their users that like to use tools to make their life easier.

@laeubi laeubi changed the title [bnd-run-maven-plugin] specifiy bndrun file in the pom.xml directly [bnd-run-maven-plugin][bnd-testing-plugin] specifiy bndrun file in the pom.xml directly Feb 4, 2023
@laeubi
Copy link
Contributor Author

laeubi commented Feb 4, 2023

For the sake of all the "what-if ..." I have here a concrete example:

This executes the JDBC TCK test as part of a regular plain maven build and the run file is as trivial as (it is even two lines longer as required due to #5539):

-runfw: org.eclipse.osgi
-tester: biz.aQute.tester.junit-platform
-runrequires: bnd.identity;id=com.microsoft.sqlserver.mssql-jdbc, \
	bnd.identity;id=org.osgi.test.cases.jdbc, \
	bnd.identity;id=junit-jupiter-engine, \
	bnd.identity;id=junit-platform-launcher

it is never modified as part of the build, but requires me to pollute the repository with an additional file that other developers most likely find irritating and won't assume it is connected to the maven config in any way.

@pkriens pkriens added the abeyance need of contributor [requires is:closed] label Feb 20, 2023
@laeubi
Copy link
Contributor Author

laeubi commented Feb 24, 2023

@pkriens you added abeyance label that says contribution is required.

I think I can contribute such option but first need to know if there is at least a chance of such changes to the maven-plugins will be accepted.

@pkriens
Copy link
Member

pkriens commented Feb 24, 2023

I am ok with this since I like those options, they simplify the build.

However, I will list to @timothyjward, @rotty3000 , etc.. since they heavily use maven.

@timothyjward
Copy link
Contributor

For the sake of all the "what-if ..." I have here a concrete example:

This executes the JDBC TCK test as part of a regular plain maven build and the run file is as trivial as (it is even two lines longer as required due to #5539):

-runfw: org.eclipse.osgi
-tester: biz.aQute.tester.junit-platform
-runrequires: bnd.identity;id=com.microsoft.sqlserver.mssql-jdbc, \
	bnd.identity;id=org.osgi.test.cases.jdbc, \
	bnd.identity;id=junit-jupiter-engine, \
	bnd.identity;id=junit-platform-launcher

it is never modified as part of the build, but requires me to pollute the repository with an additional file that other developers most likely find irritating and won't assume it is connected to the maven config in any way.

My issue with this example is that it is incomplete. A bndrun is supposed to contain the list of bundles to run. In this case the bndrun file doesn't have a -runbundles which means that it can't be used to run anything. I assume that under the covers a bndrun is created and resolved, and then that is used to run the tests?

The issue with this usage pattern is that is that it is then difficult (possibly impossible?) for me to run/rerun/debug the tests directly in my IDE. I also get no ability to see what bundles are running in the tests as it's hidden in generated files. Currently Bndtools can provide a lot of help, allowing you to Run/Debug As... > OSGi JUnit Test using the bndrun file. All this would be lost.

@pkriens
Copy link
Member

pkriens commented Feb 24, 2023

@timothyjward in 6.4 I added a cache option to the -runresolve. This makes it unnecessary to store the bundles in the bndrun file. I am using it in all my projects and it is heaven in the git merge phase :-)

@laeubi
Copy link
Contributor Author

laeubi commented Feb 24, 2023

My issue with this example is that it is incomplete. A bndrun is supposed to contain the list of bundles to run. In this case the bndrun file doesn't have a -runbundles which means that it can't be used to run anything.

Try it out, it runns the referenced test-cases and bundles mentioned in runrequires :-)

The issue with this usage pattern is that is that it is then difficult (possibly impossible?) for me to run/rerun/debug the tests directly in my IDE.

This will depend a bit on the IDE, but often it is sufficient to run things only as part of the build, i.e. in this example no one will ever install Bndtools so one can't run the tests anyways using the bndrun file.

But even if, e.g. when the bndrun file is simply written to the target folder is still can run it from there, or one simply uses Run As > Maven test...

@pkriens pkriens closed this as completed Mar 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abeyance need of contributor [requires is:closed]
Projects
None yet
Development

No branches or pull requests

4 participants