jarjar icon indicating copy to clipboard operation
jarjar copied to clipboard

Be fine with class names with '$' inside patterns

Open alexarchambault opened this issue 5 years ago • 5 comments

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 avatar Aug 06 '19 11:08 alexarchambault

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.

alexarchambault avatar Aug 06 '19 11:08 alexarchambault

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 ?

stuhood avatar Sep 23 '19 16:09 stuhood

@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,

jsirois avatar Sep 23 '19 17:09 jsirois

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?

jvican avatar Sep 24 '19 06:09 jvican

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.

stuhood avatar Sep 26 '19 04:09 stuhood