Skip to content

[CURATOR-464] update classifier and document accordingly#362

Merged
Randgalt merged 1 commit intoapache:masterfrom
bigmarvin:master
Apr 20, 2020
Merged

[CURATOR-464] update classifier and document accordingly#362
Randgalt merged 1 commit intoapache:masterfrom
bigmarvin:master

Conversation

@bigmarvin
Copy link
Copy Markdown
Contributor

@bigmarvin bigmarvin commented Apr 20, 2020

Follow-up changes of #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 conditional 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, 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. For this reason, some class-not-found error is raised in OSGi runtime when all import-package headers are fulfilled, because what's required and what's claimed required are different.

The version of imported packages wouldn't be a big concern, as it usually covers a major release if not manually specified. Even if some conflict, say different versions of same artifact are required by different bundles, that's where OSGi runtime could play its role to wire packages of different versions properly. 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 #360.

We should update the website docs (see /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.

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 this pull request may close these issues.

2 participants