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

Backward incompatibility due to the decoration around -runstartlevel #5260

Closed
amitjoy opened this issue May 24, 2022 · 9 comments · Fixed by #5263
Closed

Backward incompatibility due to the decoration around -runstartlevel #5260

amitjoy opened this issue May 24, 2022 · 9 comments · Fixed by #5263

Comments

@amitjoy
Copy link
Member

amitjoy commented May 24, 2022

In our project, we have a central bnd file where we have specified all the start levels for all the different bundles and all bndruns now inherit the start levels from this root bnd file. Now, when I run any one of them (bndrun), all the bundles specified in the start level list (-runstartlevel) are installed even though the bndrun doesn't have them in its -runbundles list.

In addition, I also have a project which uses the root -runstartlevel to prepare a metadata bundle comprising the start levels of all associated bundles.

Due to the introduction of decoration around start level (-runstartlevel), if I append * at the end of all bundles, bndruns work as expected as now * acts as optional type. But, now the aforementioned metadata bundle is unusable as it now contains a * in all the bsns.

Is it anyway possible to make the decoration around -runstartlevel optional or at least if it is possible to have it the way as it was before?

@pkriens
Copy link
Member

pkriens commented May 24, 2022

I agree this is a regression. The new decoration code changed the semantics of the old decoration code by adding literals. At least for all headers that use Project.getBundles(). I think the new semantics do make some sense but the backward incompatibility is quite bad. Having to add a '*' to each bsn is quite ugly.

Maybe we can use another character for decorators with this new behavior? I.e. a '+' for only decorating when the literal matches and '!' for matching & adding literals.

 -runbundles a, b, c
 -runbundles+ a;startlevel=1, b; startlevel=2,d;startlevel=4
 >>>>>> a;startlevel=1, b;startlevel=2, c
 -runbundles! a;startlevel=1, b; startlevel=2,d;startlevel=4
 >>>>>> a;startlevel=1, b;startlevel=2, c, d;startlevel=4

This is a very simple fix in the Project.getBundles() method.

@bjhargrave
Copy link
Member

This can be handled by making the items not a literal:

See

bnd/cnf/build.bnd

Lines 30 to 49 in cff2231

# Decorations
# We use non-literals, "{xxx}", to make sure we don't force the dependency onto
# the instruction
-buildpath+: \
"{aQute.libg}";version=project;packages="!aQute.lib.exceptions.*,*",\
"{osgi.annotation}";~version=latest;~maven-scope=provided,\
"{osgi.core}";~version=latest;~maven-scope=provided,\
"org.osgi.namespace.*";~version=latest;~maven-scope=provided,\
"org.osgi.service.*.annotations";~version=latest;~maven-scope=provided
-conditionalpackage+: \
=!aQute.lib.exceptions.*
-testpath+: \
"{aQute.libg}";version=project;packages="!aQute.lib.exceptions.*,*",\
"{osgi.annotation}";~version=latest,\
"{osgi.core}";~version=latest,\
"org.osgi.namespace.*";~version=latest,\
"org.osgi.service.*.annotations";~version=latest

The Bnd build itself relies upon the adding of literals:

bnd/cnf/build.bnd

Lines 41 to 42 in cff2231

-conditionalpackage+: \
=!aQute.lib.exceptions.*

Maybe we can use another character for decorators with this new behavior? I.e. a '+' for only decorating when the literal matches and '!' for matching & adding literals.

Interesting, but then you have to decide how + and ! merged properties are merged. :-) Do all + decorations come before/after all ! decoration.

@bjhargrave
Copy link
Member

bjhargrave commented May 24, 2022

Perhaps this is a case where that thing you did (which I am unsure how to use) to allow people to disable new features?

https://github.com/bndtools/bnd/blob/master/biz.aQute.bndlib/src/aQute/bnd/build/6.4.0.bnd

@pkriens
Copy link
Member

pkriens commented May 24, 2022

This can be handled by making the items not a literal:

This is what Amit did, we added a * at the end of the name. However, the macro with these names was used in multiple places and the second use became very hard since the * was not possible.

Interesting, but then you have to decide how + and ! merged properties are merged. :-) Do all + decorations come before/after all ! decoration.

Yeah, existential questions :-) I think we can toss a coin. This is new behavior so as long as we're consistent people can properly use it. I expect people to use one or the other. My head explodes thinking how to organize that.

Shall we do this? It is a two liner, I can fix it if you're ok with this approach.

@bjhargrave
Copy link
Member

Shall we do this? It is a two liner, I can fix it if you're ok with this approach.

What is a two-liner? I am not sure I understand your proposed change. Perhaps a call to discuss is in order.

@bjhargrave bjhargrave linked a pull request May 24, 2022 that will close this issue
bjhargrave added a commit to bjhargrave/bnd that referenced this issue May 24, 2022
Complete the changes by changing the `+` behavior back to not
adding literals.

Fixes bndtools#5260

Signed-off-by: BJ Hargrave <[email protected]>
@bjhargrave bjhargrave linked a pull request May 24, 2022 that will close this issue
@amitjoy
Copy link
Member Author

amitjoy commented May 24, 2022

Thanks a lot @bjhargrave and @pkriens to take care of it so carefully 👍

@bjhargrave
Copy link
Member

@amitjoy Please confirm the latest 6.4.0-SNAPSHOT fixes this for you and I will then push the fix to a new 6.3.0 RC build.

@amitjoy
Copy link
Member Author

amitjoy commented May 25, 2022

@bjhargrave on it

@amitjoy
Copy link
Member Author

amitjoy commented May 25, 2022

@bjhargrave I am hereby confirming that the behaviour is once again backward compatible and works as expected. Thanks a lot once again for your quick support as always 👍

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

Successfully merging a pull request may close this issue.

3 participants