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

Proguard contrib module #972

Merged
merged 9 commits into from
Nov 2, 2020
Merged

Proguard contrib module #972

merged 9 commits into from
Nov 2, 2020

Conversation

sbly
Copy link
Contributor

@sbly sbly commented Oct 5, 2020

Create Proguard contrib module

This module will run proguard on the normal assembly output jar to shrink it. By default all steps - shrink, optimize, obfuscate, and preverify - are run though this can be configured. The default entrypoint is finalMainClass. Any additional options can be specified freely with additionalOptions (proguard has many).

@sbly sbly changed the title Proguard Proguard contrib module Oct 5, 2020
Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution. The module looks ok to me. I added some comments I'd like you to address.

Also to accept this new contrib module, we need some documentation under docs/pages/9 - Contrib Modules.md (modules in alphabetical order). Having some Scaladoc for all public targets would greatly help later users of that module too.

build.sc Outdated Show resolved Hide resolved
contrib/proguard/src/Proguard.scala Outdated Show resolved Hide resolved
contrib/proguard/src/Proguard.scala Outdated Show resolved Hide resolved
Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

For consistency, if you use an library from an explicit Java version (via JAVA_HOME) you should also call the Java executable from this specific version.

build.sc Outdated Show resolved Hide resolved
build.sc Outdated Show resolved Hide resolved
@sbly
Copy link
Contributor Author

sbly commented Oct 6, 2020

@lefou Thanks for the review! I've addressed most of your feedback. The one thing I'm a little confused about is this:

if you use an library from an explicit Java version (via JAVA_HOME) you should also call the Java executable from this specific version.

Does Mill use JAVA_HOME or does it get the standard library from elsewhere? I'm having trouble figuring out how the code is compiled in the first place

@lefou
Copy link
Member

lefou commented Oct 6, 2020

@lefou Thanks for the review! I've addressed most of your feedback. The one thing I'm a little confused about is this:

if you use an library from an explicit Java version (via JAVA_HOME) you should also call the Java executable from this specific version.

Does Mill use JAVA_HOME or does it get the standard library from elsewhere? I'm having trouble figuring out how the code is compiled in the first place

Mill uses JAVA_HOME, but I was referring to the call out to the java executable in the proguard target. It's not necessarily the same as $JAVA_HOME/bin/java and therefore problematic.

val outJar = PathRef(T.dest / "out.jar")

val cmd = os.proc(
"java",
Copy link
Member

Choose a reason for hiding this comment

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

Better use the java executable found under javaHome, to make it consistent with the later used classpath.

@sbly
Copy link
Contributor Author

sbly commented Oct 7, 2020

Thanks for the help. I switched to using the java binary under JAVA_HOME, and also added documentation.

Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

Looks good to me. Do you plan to also add some ScalaDoc and/or tests anytime soon? If not I can merge now, but those additions would help greatly in comsumption and maintenance of this contrib module.

@sbly
Copy link
Contributor Author

sbly commented Oct 8, 2020

@lefou I can add some ScalaDoc. I was testing using just the scratch/folder. I wasn't sure how to write automated tests for this but I can look at some examples and try to do it. We can hold off on merging for now

Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

Scaladoc looks good. I found two other subtle improvements that are specific to mills caching.

Comment on lines 49 to 51
def javaHome: T[PathRef] = T {
PathRef(Path(System.getProperty("java.home")))
}
Copy link
Member

Choose a reason for hiding this comment

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

We need to mark this target as input, otherwise mills caching would hide potential changes of the system property.

Suggested change
def javaHome: T[PathRef] = T {
PathRef(Path(System.getProperty("java.home")))
}
def javaHome: T[PathRef] = T.input {
PathRef(Path(System.getProperty("java.home")))
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'll make the change. But I'm wondering now if this should actually be Sources? What's the difference?

Copy link
Member

Choose a reason for hiding this comment

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

Sources and also Source are specializations of Input. Also Source(s) support the watch mode (mill -w), whereas Input is not able to trigger a re-build on it's own.

contrib/proguard/src/Proguard.scala Outdated Show resolved Hide resolved
contrib/proguard/src/Proguard.scala Outdated Show resolved Hide resolved
@sbly
Copy link
Contributor Author

sbly commented Nov 2, 2020

@lefou Hi, I finally had some time to work on this again. I added some basic tests. I am ready to merge this now if that's OK with you

@lefou lefou merged commit f555644 into com-lihaoyi:master Nov 2, 2020
@lefou
Copy link
Member

lefou commented Nov 2, 2020

Thank you @sbly !

@lefou lefou added this to the after 0.8.0 milestone Nov 2, 2020
@sbly sbly deleted the proguard branch November 2, 2020 20:24
@lefou
Copy link
Member

lefou commented Nov 27, 2020

I just notices, that our test setup wasn't running the proguard tests. Running them locally gives me the following error:

[263/263] contrib.proguard.test.testCached 
-------------------------------- Running Tests --------------------------------
mill.contrib.proguard.ProguardTests: Using experimental parallel evaluator with 4 threads
mill.contrib.proguard.ProguardTests: [#1] [2/2] proguardClasspath 
+ mill.contrib.proguard.ProguardTests.proguard.should download proguard jars 1504ms  
mill.contrib.proguard.ProguardTests: Using experimental parallel evaluator with 4 threads
mill.contrib.proguard.ProguardTests: [#0] [40/52] libraryJars 
mill.contrib.proguard.ProguardTests: [#3] [9/52] mill.scalalib.ZincWorkerModule.classpath failed
mill.contrib.proguard.ProguardTests: [#2] [41/52] mill.scalalib.ZincWorkerModule.worker failed
mill.contrib.proguard.ProguardTests: [#2] [42/52] compile failed
mill.contrib.proguard.ProguardTests: [#2] [43/52] finalMainClassOpt failed
mill.contrib.proguard.ProguardTests: [#1] [44/52] localClasspath failed
mill.contrib.proguard.ProguardTests: [#3] [45/52] prependShellScript failed
mill.contrib.proguard.ProguardTests: [#0] [47/52] manifest failed
mill.contrib.proguard.ProguardTests: [#2] [46/52] finalMainClass failed
mill.contrib.proguard.ProguardTests: [#1] [48/52] upstreamAssembly failed
mill.contrib.proguard.ProguardTests: [#2] [49/52] entryPoint failed
mill.contrib.proguard.ProguardTests: [#1] [50/52] assembly failed
mill.contrib.proguard.ProguardTests: [#1] [51/52] inJar failed
mill.contrib.proguard.ProguardTests: [#1] [52/52] proguard failed
X mill.contrib.proguard.ProguardTests.proguard.create proguarded jar 1741ms 
  scala.MatchError: Left(Failure(
  Resolution failed for 1 modules:
  --------------------------------------------
    com.lihaoyi:mill-scalalib-worker_2.13:0.9.3-20-ea5b88 
        not found: /home/lefou/.ivy2/local/com.lihaoyi/mill-scalalib-worker_2.13/0.9.3-20-ea5b88/ivys/ivy.xml
        not found: https://repo1.maven.org/maven2/com/lihaoyi/mill-scalalib-worker_2.13/0.9.3-20-ea5b88/mill-scalalib-worker_2.13-0.9.3-20-ea5b88.pom
  ,None)) (of class scala.util.Left)
    mill.contrib.proguard.ProguardTests$.$anonfun$tests$7(ProguardTests.scala:44)
    mill.contrib.proguard.ProguardTests$.$anonfun$tests$7$adapted(ProguardTests.scala:43)
    mill.contrib.proguard.ProguardTests$.workspaceTest(ProguardTests.scala:33)
    mill.contrib.proguard.ProguardTests$.$anonfun$tests$6(ProguardTests.scala:36)
1 targets failed
contrib.proguard.test.testCached mill.contrib.proguard.ProguardTests mill.contrib.proguard.ProguardTests.proguard.create proguarded jar

@sbly
Copy link
Contributor Author

sbly commented Dec 17, 2020

Hi @lefou. Sorry about being super late getting to this, I don't regularly check Github when I'm not actively working on a PR.

So locally, I would always run mill -i all __.publishLocal before testing. If I didn't, I got the same error as you above. I thought this was expected and was just the way mill's tests were set up, so I didn't mention it. I just tried again and it still works.

If this is not the way tests are expected to work, I am happy to work out the error with you. I based my tests off of the existing tests, so there was some copy-pasting that may be the culprit.

lefou added a commit to lefou/mill that referenced this pull request Dec 17, 2020
@lefou
Copy link
Member

lefou commented Dec 17, 2020

@sbly Thanks for making your assumptions clear. All mill tests should run as-is. I created a PR to fix the setup.

lefou added a commit to lefou/mill that referenced this pull request Dec 17, 2020
lefou added a commit that referenced this pull request Dec 18, 2020
Test was introduced in PR #972.

* Added contrib.proguard test to test suite
* Addressed compiler warnings
* Accept specific test failures
* Catch more exceptions/errors

Pull request: #1080
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