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

[via GitHub]

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-unsubscr...@mina.apache.org

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


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