Skip to content

Conversation

@jzhuge
Copy link
Member

@jzhuge jzhuge commented Aug 18, 2020

Skip building the empty jars that we don't need.

Verified the shadow jars still built and no empty jars.

@rdblue rdblue merged commit d2676eb into apache:master Aug 18, 2020
@rdblue
Copy link
Contributor

rdblue commented Aug 18, 2020

Looks great! Thanks @jzhuge!

cmathiesen pushed a commit to ExpediaGroup/iceberg that referenced this pull request Aug 19, 2020
cmathiesen pushed a commit to ExpediaGroup/iceberg that referenced this pull request Aug 19, 2020
@openinx
Copy link
Member

openinx commented Aug 20, 2020

@jzhuge Did you find that the intellij IDE could not import the relocated guava classes after applied this patch ? Once applied this patch in my own repo, the relocated package (such as org.apache.iceberg.relocated.com.google.common.base.Preconditions ) could not be imported, but I could run unit tests in my ide and build the project under my host.

@openinx
Copy link
Member

openinx commented Aug 20, 2020

The simple pr (#1360) fixed my package import issue, but i'm not familiar with gradle building, is it the right direction to fix this issue ?

@jzhuge
Copy link
Member Author

jzhuge commented Aug 20, 2020 via email

@openinx
Copy link
Member

openinx commented Aug 20, 2020

There's a GuavaClasses inside the empty jar. Without that class, then bundled-guava won't reference any classes from guava ? finally, the relocate won't have effect in this module. But I don't understand why could we build this project in terminal.

public class GuavaClasses {

  /*
   * Referencing Guava classes here includes them in the minimized and relocated Guava jar
   */
  static {
    VisibleForTesting.class.getName();
    Joiner.class.getName();
    MoreObjects.class.getName();
    Objects.class.getName();
    Preconditions.class.getName();
    Splitter.class.getName();
    Throwables.class.getName();
    BiMap.class.getName();
    FluentIterable.class.getName();
    ImmutableBiMap.class.getName();
    ImmutableList.class.getName();
    ImmutableMap.class.getName();
    ImmutableSet.class.getName();
    Iterables.class.getName();
    Iterators.class.getName();
    ListMultimap.class.getName();
    Lists.class.getName();
    MapMaker.class.getName();
    Maps.class.getName();
    Multimap.class.getName();
    Multimaps.class.getName();
    Ordering.class.getName();
    Sets.class.getName();
    Streams.class.getName();
    Hasher.class.getName();
    HashFunction.class.getName();
    Hashing.class.getName();
    Files.class.getName();
    Bytes.class.getName();
    MoreExecutors.class.getName();
    ThreadFactoryBuilder.class.getName();
    Iterables.class.getName();
  }

}

image

@jzhuge
Copy link
Member Author

jzhuge commented Aug 20, 2020 via email

@rdblue
Copy link
Contributor

rdblue commented Aug 20, 2020

Looks like IntelliJ relies on the 'empty' classifier somehow. Probably not worth the trouble to find out why when we can just add the classifier back. I've updated the PR to use archiveClassifier and will merge when it passes tests.

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.

3 participants