[GitHub] [mina-sshd] HannesWell commented on a diff in pull request #343: Simplify maven-bundle-plugin configuration

["HannesWell (via GitHub)" <gi...@apache.org>]

HannesWell commented on code in PR #343:
URL: https://github.com/apache/mina-sshd/pull/343#discussion_r1145421560


##########
pom.xml:
##########
@@ -1445,31 +1445,20 @@
                 <inherited>true</inherited>
                 <extensions>true</extensions>
                 <executions>
-                    <execution>
-                        <id>versions</id>
-                        <phase>validate</phase>
-                        <goals>
-                            <goal>cleanVersions</goal>
-                        </goals>
-                        <configuration>
-                            <versions>
-                                <sshd.osgi.version.clean>${project.version}</sshd.osgi.version.clean>
-                            </versions>
-                        </configuration>
-                    </execution>
                     <execution>
                         <id>bundle-manifest</id>
                         <phase>process-classes</phase>
                         <goals>
                             <goal>manifest</goal>
                         </goals>
                         <configuration>
+                            <supportIncrementalBuild>true</supportIncrementalBuild>
                             <instructions>
                                 <Import-Package><![CDATA[
-                                  org.apache.sshd*;version="[$(version;==;${sshd.osgi.version.clean}),$(version;=+;${sshd.osgi.version.clean}))",
+                                  org.apache.sshd*;version="${range;[==,=+);${maven_version;${project.version}}}",

Review Comment:
   > However: I just noticed that here (and in the slf4j line below) we have an unholy mixture of bnd macro references and maven property references. What happens if someone defines a maven property named "range"?
   
   Good point and goo question. But because after range there is some more I think the maven properties substitution would not interpret this as range variable. But I'm not sure about that and better save than sorry. :)
   
   > bnd allows other brackets to be used. Can we please use `$(...)` for bnd, and reserve `${...}` for maven? Or use `$${macro...}` for bnd references; see [the maven-bundle-plugin documentation](https://felix.apache.org/documentation/_attachments/components/bundle-plugin/manifest-mojo.html#instructions).
   
   I changed it to angular brackets now so that the variable/macro end is not confused with a inklusive/exclusive range bound `)` or `]`. This is the list of all allowed types:
   https://bnd.bndtools.org/chapters/850-macros.html section 35.1 Macro patterns
   
   > Only tangentially related to this change: I wonder if ==,=+ is good. Apache MINA sshd has an unfortunate habit of breaking API in minor versions, so it might actually be safer to require exact matches, and not a range at all... something like `$(version;===;$(maven_version;${project.version}))`. Or at least use a range [===;=+), which is what the Eclipse Orbit re-bundling uses.
   
   As you prefer :) But a like `bundle-version="1.2.3"` only defines a lower bound, what would make it even worse if MINA does not follow Semantic Versioning. I now changed it to `[===;=+)`, but if you only want to allow that exact version `[===;===]` would probably the way to go, which leads to ranges like `bundle-version="[1.2.3,1.2.3]"`. Theoretically there is still some space for different qualifiers, but if you don't use them practicality it does not allow something else.
   
   By the way in the Eclipse Platform we are now using Maven-Targets to obtain artifacts from Maven-Central directly that already have proper OSGi metadata built in, like MINA SSHD, and try to fade out Orbit:
   https://github.com/eclipse-platform/eclipse.platform.releng.aggregator/blob/d2ebbe9ee4f98f9b0c8423b3df1e5a42544ff925/eclipse.platform.releng.prereqs.sdk/eclipse-sdk-prereqs.target#L171-L176
   
   That's the reason why I reached out to you with my previous PR. :)
   https://github.com/eclipse-platform/eclipse.platform.releng.aggregator/pull/981



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org