Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Relocating external dependencies in fat jar #27

Merged
merged 1 commit into from
Dec 17, 2016
Merged

Relocating external dependencies in fat jar #27

merged 1 commit into from
Dec 17, 2016

Conversation

tomasbjerre
Copy link
Contributor

  • To avoid classpath collisions.

@cdancy cdancy added this to the v0.0.11 milestone Dec 17, 2016
@cdancy
Copy link
Owner

cdancy commented Dec 17, 2016

This is fine but lets move to <project-root>/gradle/additional-artifacts.gradle where I've already stubbed out this task.

Lets do the below instead. Think that should cover all pulled in deps and give a more expected shaded naming convention.

shadowJar {
     relocate 'com.google', 'com.cdancy.bitbucket.rest.shaded.google'
     relocate 'com.squareup', 'com.cdancy.bitbucket.rest.shaded.squareup'
     relocate 'autovalue', 'com.cdancy.bitbucket.rest.shaded.autovalue'
     relocate 'javax', 'com.cdancy.bitbucket.rest.shaded.javax'
     relocate 'net', 'com.cdancy.bitbucket.rest.shaded.net'
     relocate 'okio', 'com.cdancy.bitbucket.rest.shaded.okio'
     relocate 'org', 'com.cdancy.bitbucket.rest.shaded.org'
 }

@cdancy cdancy self-assigned this Dec 17, 2016
@tomasbjerre
Copy link
Contributor Author

So se., ch. and bsh.* should not be relocated?

@cdancy
Copy link
Owner

cdancy commented Dec 17, 2016

I don't see either of those in the -all jar. Can you paste the contents of your build/libs/*-all.jar here? Perhaps they are different then mine. unzip -l *-all.jar

@tomasbjerre
Copy link
Contributor Author

You are right =) Its not in there!

@cdancy
Copy link
Owner

cdancy commented Dec 17, 2016

Edited the code snippet above to add a shaded package to make it absolutely clear these packages are shaded.

@@ -1,5 +1,15 @@
shadowJar {
mergeServiceFiles()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any particular reason you removed this line or just a typo?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added it again!

@@ -8,6 +8,8 @@ out
# OS generated files #
######################
.DS_Store
src/generated
test-output
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume these are maven generated folders? Are you using maven somehow with this project?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was probably just some files laying around. Originally generated by Eclipse. But I see Eclipse support was removed in fd13b17#diff-c197962302397baf3a4cc36463dce5ea maby you just relocate the libraries yourself?

}

artifacts {
archives shadowJar
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we including this artifacts block? Shouldn't be necessary as we already define this within <project-root>/gradle/publishing.gradle

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

 * To avoid classpath collisions.
@cdancy
Copy link
Owner

cdancy commented Dec 17, 2016

I don't mind you adding in the maven/generated dirs. Can't remember exactly why I ripped that eclipse code out initially but I remember it causing problems within my workspace.

In either event: feel free to add the generated dirs to the .gitignore if it helps.

@tomasbjerre
Copy link
Contributor Author

Ok I'll remember it if I need something else changed in this project! But it should be ready to be merged now right?

@cdancy cdancy merged commit d080486 into cdancy:master Dec 17, 2016
@cdancy
Copy link
Owner

cdancy commented Dec 17, 2016

Merged. @tomasbjerre thanks for the PR!

FYI: not sure if you're looking for a new version to be put out with this change but I planned on doing another release by end of weekend. Waiting for 1 more expected PR to come in from a dev and then will release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants