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.

Sign up for GitHub

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

Jump to bottom

Improve code templates #5907

Merged
merged 10 commits into from
Dec 1, 2023
Merged

Improve code templates #5907

merged 10 commits into from
Dec 1, 2023

Conversation

chrisrueger
Copy link
Contributor

@chrisrueger chrisrueger commented Nov 28, 2023 •
edited
Loading

Related to bndtools/bndtools.workspace.min#11 (comment)

It tries to address (some) of the suggestions by @pkriens from here:

If we update the templates (and we should) some thinks to consider:

  • Specify the framework without version, let bnd pick one

addressed in this PR

  • You might want to make the bndrun template use cache. This works great in general and saves lots of problems with git merge.

@pkriens do you have some pointer what that means? Do you mean https://bnd.discourse.group/t/the-resolve-cache-mode/302 ?

  • No versions, use default version and let the repositories control the versions avaialble

addressed in this PR. Let me know if I forgot something

  • Remove Bundle-Version everywhere, should be defined in build.bnd

addressed in this PR

  • No specification of java anywhere

addressed in this PR

  • No Export-Package or Private-Package headers, defaults should be used.

addressed in this PR (not sure I got everything)

  • Need to update our Junit storyhere. Probably need to work with @kriegfr

TODO

  • remove the enroute stuff

TODO

  • Might throw out some

TODO

This was referenced Nov 28, 2023
Merged
Closed
@chrisrueger chrisrueger marked this pull request as ready for review November 29, 2023 10:20
pkriens
pkriens reviewed Nov 30, 2023
View reviewed changes
Copy link
Member

@pkriens pkriens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, I think we should remove version=latest everywhere.

bnd selects version=lowest for compiling and version=latest for running. This is a good strategy we should not overide.

I also think we should remove the comments everywhere, except a short description at the top.

org.bndtools.templates.osgi/resources/templates/ds/bnd.bnd Outdated Show resolved Hide resolved
org.bndtools.templates.osgi/resources/templates/ds/launch.bndrun Outdated Show resolved Hide resolved
org.bndtools.templates.osgi/resources/templates/integration-test/bnd.bnd Outdated
@@ -1,9 +1,9 @@
# This is the version of JUnit that will be used at build time and run time
junit: org.apache.servicemix.bundles.junit;version="[4.11,5)"
junit: org.apache.servicemix.bundles.junit;version=latest
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kriegfrj can you give advise for the templates how to use JUnit 5?

Personally I am ok with 4 since it is so much simpler and smaller. However, you've been advocating JUnit 5 a lot so should the templates be migrated?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chrisrueger I think we should refer to junit as a macro so we do not have this information in all bndrun files.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pkriens

@chrisrueger I think we should refer to junit as a macro so we do not have this information in all bndrun files.

I have added a line like this junit: org.apache.servicemix.bundles.junit to build.bnd of the workspace template (next door).
This how it can be used like ${junit} right?

org.bndtools.templates.osgi/resources/templates/integration-test/bnd.bnd Outdated Show resolved Hide resolved
org.bndtools.templates.osgi/resources/templates/integration-test/bnd.bnd Outdated Show resolved Hide resolved
org.bndtools.templates.osgi/resources/templates/integration-test/bnd.bnd Show resolved Hide resolved
org.bndtools.templates.osgi/resources/templates/integration-test/bnd.bnd Outdated Show resolved Hide resolved
org.bndtools.templates.osgi/resources/templates/launch/felix-gogo/$fileName$
@@ -1,5 +1,4 @@
-runfw: org.apache.felix.framework;version=4
-runee: JavaSE-1.8
-runfw: org.apache.felix.framework
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again, move to build.bnd

@pkriens
Copy link
Member

pkriens commented Nov 30, 2023

Also remove any references to R8, this way for R9 we do not have to change it :-)

@pkriens
Copy link
Member

pkriens commented Nov 30, 2023

Great work ...

@chrisrueger
remove char
4d04c91 
Signed-off-by: Christoph Rueger <chrisrueger@gmail.com>

Update template files

- remove versions where possible

Signed-off-by: Christoph Rueger <chrisrueger@gmail.com>
@chrisrueger
remove -runee: JavaSE-$javaLevel$
fb3ebb4 
Signed-off-by: Christoph Rueger <chrisrueger@gmail.com>
@chrisrueger
remove javac stuff
8d8edb7 
Signed-off-by: Christoph Rueger <chrisrueger@gmail.com>
@chrisrueger
Remove headers
c7aa094 
No Export-Package or Private-Package headers, defaults should be used.

Signed-off-by: Christoph Rueger <chrisrueger@gmail.com>
@chrisrueger
use -resolve: cache
36e2638 
Signed-off-by: Christoph Rueger <chrisrueger@gmail.com>
@chrisrueger
Remove runee
277d2cb 
Signed-off-by: Christoph Rueger <chrisrueger@gmail.com>
@chrisrueger chrisrueger force-pushed the improve-code-templates branch from 45f0ab6 to 277d2cb Compare November 30, 2023 15:33
@pkriens
Copy link
Member

pkriens commented Nov 30, 2023

Merge?

@chrisrueger
Copy link
Contributor Author

Merge?

No, I would add your review suggestions first. Try to be ready tomorrow.

@chrisrueger chrisrueger marked this pull request as draft November 30, 2023 19:05
@chrisrueger
Implement suggestions of review
4c3d8c4 
- remove version=latest
- remove references to R8
- only include osgi.core

Signed-off-by: Christoph Rueger <chrisrueger@gmail.com>
@chrisrueger
add back annotations
a2533dc 
They are required by the sources

Signed-off-by: Christoph Rueger <chrisrueger@gmail.com>
@chrisrueger
remove runee
f0b3c06 
Signed-off-by: Christoph Rueger <chrisrueger@gmail.com>
@chrisrueger chrisrueger mentioned this pull request Nov 30, 2023
Closed
@chrisrueger
Copy link
Contributor Author

@pkriens I wanted to test this with an Eclipse instance started from my bndtools Dev Workspace. But then I hit #5911

Maybe we can talk on slack how to test this. I have the feeling I am missing some pieces of the puzzle.

@chrisrueger
Use juni + mockito macros defined in workspace template
5c9eaf6 
Signed-off-by: Christoph Rueger <chrisrueger@gmail.com>
@chrisrueger chrisrueger marked this pull request as ready for review December 1, 2023 09:25
@chrisrueger
Copy link
Contributor Author

@pkriens I "think" I am done for now. Feel free to merge. These changes depend on bndtools/bndtools.workspace.min#11

I had some trouble testing, so let me know if merging is ok for now or we should have a chat about it.

@pkriens pkriens merged commit 253ae2b into bndtools:master Dec 1, 2023
7 checks passed
@pkriens
Copy link
Member

pkriens commented Dec 1, 2023

let's merge so I can easier take a look.

@chrisrueger
Copy link
Contributor Author

Thanks 👍

@chrisrueger chrisrueger mentioned this pull request Dec 2, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pkriens pkriens pkriens left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@chrisrueger @pkriens