[GitHub] [curator] bigmarvin opened a new pull request #362: [CURATOR-464] update classifier and document accordingly

[GitBox <gi...@apache.org>]

bigmarvin opened a new pull request #362:
URL: https://github.com/apache/curator/pull/362


   Follow-up changes of https://github.com/apache/curator/pull/355, to address the remaining concerns raised inside.
   
   _Why do we need ant and build helper plugins. Can't install-file from the install plugin be used?_
   
   There's some slight difference between. The helper plugin attaches a new artifact to the project, and this info could be consumed by any execution of plugins afterwards. For instance, the install plugin is not explicitly told to install another file, and yet it does so by learning there's more than 1 artifact in this project. If nobody else consumes that info, the goal `attach-artifact` and `install-file` could work interchangeably.
   
   I tried to engage `install-file`, on the other hand, and it didn't work well. The primary problem would be it doesn't support conditionally install, so I would have to configure `install-file` in concrete projects when necessary. This kind of asymmetry, that `shade` is configured globally in parent project while `install-file` is configured in concrete projects, does concern me compared with `attach-artifact` based approach.
   
   _If I understand the need correctly this only needs to be done on the curator-client module right? That's where the shading happens. Or is that you need versions of the other modules that don't refer to the shaded Guava? Will that actually work? That code is all compiled assuming a particular version of Guava. I guess if OSGI-Curator users use the right version of Guava it would work._
   
   This change is required for projects consuming relocated guava packages, so it would be more than curator-client only. The root cause of [CURATOR-464](https://issues.apache.org/jira/browse/CURATOR-464), from my perspective, would be the shade plugin not working on OSGi headers in manifest, or at least the package relocation part doesn't work. The relocation does 1) relocate the guava packages inside curator-client and 2) change the byte code so imports of guava packages become imports of relocated guava packages, but it fails to update the import/export-package headers in manifest, so some class-not-found error is raised in OSGi runtime when all import-package headers are fulfilled.
   
   The version of imported packages wouldn't be a big concern, as it usually covers a major release if not manually specified. I'm attaching the reformatted import-package header from curator-framework, where the version of imported package could be found.
   ```
   Import-Package:
     org.apache.zookeeper;version="[3.4,4.0)",
     org.apache.zookeeper.admin;version="[3.4,4.0)",
     org.apache.zookeeper.data;version="[3.4,4.0)",
     org.apache.zookeeper.proto;version="[3.4,4.0)",
     org.apache.zookeeper.server;version="[3.4,4.0)",
     org.apache.zookeeper.server.quorum;version="[3.4,4.0)",
     org.apache.zookeeper.server.quorum.flexible;version="[3.4,4.0)",
     com.fasterxml.jackson.databind;version="[2.10,3)",
     com.google.common.base;version="[27.0,28)",
     com.google.common.cache;version="[27.0,28)",
     com.google.common.collect;version="[27.0,28)",
     org.apache.curator;version="[5.0,6)",
     org.apache.curator.drivers;version="[5.0,6)",
     org.apache.curator.ensemble;version="[5.0,6)",
     org.apache.curator.ensemble.fixed;version="[5.0,6)",
     org.apache.curator.utils;version="[5.0,6)",
     org.apache.jute,org.slf4j;version="[1.7,2)"
   ```
   
   _I'm not convinced that original is the right prefix for the unshaded JAR. Is there a standard prefix for these things? I'd appreciate it if you'd search around and show some other projects that are doing this and show what prefixes they use._
   
   It's renamed into osgi, essentially same as https://github.com/apache/curator/pull/360.
   
   _We should update the website docs (see <root>/src/site) to detail these additional JARs. That should be part of this PR._
   
   Good point. I've tried to compose some in this change, but please feel free to update it if anything.


----------------------------------------------------------------
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.

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