pantsbuild / jarjar

An export of https://code.google.com/p/jarjar/ @ svn:r142 for pants tool use and further development.
Apache License 2.0
37 stars 27 forks source link

Be fine with class names with '$' inside patterns #50

Open alexarchambault opened 5 years ago

alexarchambault commented 5 years ago

This PR allows to have $ signs appear in class names inside patterns, like

Rule rule = new Rule();
rule.setPattern("org.something.Test$1");
rule.setResult("shaded.@0");

(renames org.something.Test$1 to shaded.org.something.Test$1).

It cautiously escapes patterns in org.pantsbuild.jarjar.Wildcard, so that these are handled fine by regexes down the line.

Fixes https://github.com/pantsbuild/jarjar/issues/23.

alexarchambault commented 5 years ago

Not sure what the CI errors correspond to:

11:33:33 00:49         [jar-tool]

                   compile(src/test/java/org/pantsbuild/jarjar/integration:base) failed: u'sun.boot.class.path'

11:35:07 02:23         [cache] 

                   compile(src/main/java/org/pantsbuild/jarjar:jarjar) failed: u'sun.boot.class.path'

                     No cached artifacts for 1 target.

                     Invalidated 1 target.

11:35:07 02:23         [jar-tool]

                   compile(src/test/java/org/pantsbuild/jarjar/example:example) failed: u'sun.boot.class.path'

FAILURE: Compilation failure: Failed jobs: compile(src/test/java/org/pantsbuild/jarjar/example:example), compile(src/main/java/org/pantsbuild/jarjar:jarjar), compile(src/test/java/org/pantsbuild/jarjar/integration:base)

./pants test :: runs fine locally, with a JDK 8.

stuhood commented 5 years ago

It looks like this needs a fix in master. From CI:

$ jdk_switcher use ${JDK}

./pants --compile-zinc-worker-count=${WORKERS} doc test ::

jdk_switcher: command not found

...so most likely we're getting JDK9+ here (whatever the default is). The build hasn't been touched here in too long.

@jsirois , @benjyw : Thoughts on hardening the fork a bit more by moving it into pantsbuild/pants ?

jsirois commented 5 years ago

@jsirois , @benjyw : Thoughts on hardening the fork a bit more by moving it into pantsbuild/pants ?

I'd be happy with that. A side-benefit being elimination of the occasional issue we get filed for out-of-scope-for-pants work despite the org name. Tucked away in the pants repo no-one not using Pants would be likely to find it,

jvican commented 5 years ago

We’re depending on this artifact to shade projects in sbt and I’d like to improve jar jar to shade symbol names in ScalaSig javac annotations so that there’s finally a library that allows sharing libraries. It would be simpler if I can make and release those improvements outside of the pants repo. Would it be possible we merge this PR here and continue to maintain it separately to pants?

stuhood commented 5 years ago

It would be simpler if I can make and release those improvements outside of the pants repo.

Hm. Would it be? We have better docs for publishing from the pantsbuild/pants repo than this one: https://www.pantsbuild.org/release_jvm.html

Would it be possible we merge this PR here and continue to maintain it separately to pants?

The PR is currently broken, because the build in this repo is broken, because travis has some amount of upkeep and non-reproducibility. @jsirois laid some groundwork for fixing things in #43, but in general, avoiding maintaining multiple builds is easiest.

I'm hackweeking this week, and so won't have time to do anything about this. But if you'd like to help, a modernized version of #43 would be awesome.