Upgrade to SBT 1.5 #42

sbly · 2021-08-11T22:05:12Z

No description provided.

SethTisue · 2021-08-12T18:57:40Z

Was it hard keeping sbt 0.13 support? (Because I don't think it's really necessary to keep sbt 0.13 in the crossbuild.)

sbly · 2021-08-12T19:14:35Z

@SethTisue Uh whoops, I forgot about that, sorry! I was only testing 1.5.5. I can fix it to work with both versions, but it would easier if we just remove it, so I'll do that if you're OK with it

SethTisue · 2021-08-12T19:52:45Z

Yes, definitely okay. We don't have surplus of maintenance energy in this repo, so I think it's good to prioritize level-of-maintenance-effort over supporting people using ancient versions of things. (And sbt 0.13 is really ancient at this point; I'm encouraging basically all sbt plugin authors to stop supporting it. If there's somebody who's truly stuck on sbt 0.13 and absolutely needs to make changes to this plugin, they're welcome to fork the repo and do the maintenance and publishing work themselves.)

SethTisue · 2021-08-12T19:58:42Z

I hit the "migrate" button over at travis-ci.com to get CI going again. (And close/reopen to kick it off.)

sbly · 2021-08-12T20:19:12Z

@SethTisue Thanks, I'll remove it!

It looks like we do have Travis CI. Did you want to migrate to Github actions?

SethTisue · 2021-08-13T00:19:26Z

Staying on Travis CI is fine, but moving to GitHub Actions is fine too, if somebody wants to tackle that.

sbly · 2021-08-13T14:41:18Z

@SethTisue Gotcha. It looks like it's passing. Can we merge this for now? And then cut a new release?

SethTisue · 2021-08-13T22:29:37Z

@pvlugter @mpollmeier are you guys still interested in this plugin? I can temporarily step in from time to time in sbt/* repos, but there are so many such repos that there's a danger of it eating too much of my time

mpollmeier · 2021-08-14T16:04:04Z

Hmm, we do use this plugin (ancient version still works fine). I don't have enough free cycles to actively maintain this repo, but helping out here and there (if it's within my area of knowledge) may work. Not sure if that's what you were after?

pvlugter · 2021-08-16T04:11:09Z

We haven't used this plugin at Lightbend for a long time now. Would be useful to have the releasing updated to be an automated publish on tags, so that it's easy for any active maintainers to review and release changes.

pvlugter · 2021-08-16T04:13:18Z

src/main/scala/com/typesafe/sbt/SbtProguard.scala

+    proguardInputs := {
+      assembly.value
+      Seq((assembly / assemblyOutputPath).value)
+    },


Is there a reason for depending on sbt-assembly and always going through an assembly first? Just using the classpath seems like a simpler default, and pre-assembly can be added when needed?

I couldn't get it to work by using the classpath. My understanding (correct me if i'm wrong) is that Proguard can only work on fat jars. But maybe I was doing something wrong

Just tried it again and it's still running into errors. It's possible that the task keys this was using before have changed between versions

Ok. Since there are other changes, both inputs and libraries, could be good to confirm what's going on here. It should certainly work as before, with a classpath of inputs rather than a fat jar.

I'm really not a proguard expert but would also think that sbt-assembly shouldn't be mandated.
It wouldn't be the end of the world and if there's no other way I'd rather merge this PR.

However: I just tried your branch and reverted to the old proguardInputs := (fullClasspath in Runtime).value.files and removed all sbt-assembly related settings/dependency declarations. The included test projects work just fine (two of which do not use sbt-assembly themselves), and so does our internal build (which uses sbt-native-packager).

Can you clarify what kind of errors you're running into? Maybe those are actually unrelated to assembly?

That's odd. None of the tests work for me when I try that. This is the error I get:

[info] [info] Proguard command: [info] [info] java -Xmx256M -cp /Users/sbly/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/com/guardsquare/proguard-base/7.0.0/proguard-base-7.0.0.jar:/Users/sbly/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/com/guardsquare/proguard-core/7.0.0/proguard-core-7.0.0.jar:/Users/sbly/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/com/google/code/gson/gson/2.8.5/gson-2.8.5.jar:/Users/sbly/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/jetbrains/kotlin/kotlin-stdlib/1.3.31/kotlin-stdlib-1.3.31.jar:/Users/sbly/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/jetbrains/kotlin/kotlin-stdlib-common/1.3.31/kotlin-stdlib-common-1.3.31.jar:/Users/sbly/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/jetbrains/kotlinx/kotlinx-metadata-jvm/0.1.0/kotlinx-metadata-jvm-0.1.0.jar:/Users/sbly/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/jetbrains/annotations/13.0/annotations-13.0.jar proguard.ProGuard -injars "/private/var/folders/34/jl47bvtd2_14y99v6gx4k0bm0000gt/T/sbt_d73d4659/target/scala-2.12/classes" -injars "/Users/sbly/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/scala-lang/scala-library/2.12.3/scala-library-2.12.3.jar" -libraryjars "/Users/sbly/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/scala-lang/scala-library/2.12.3/scala-library-2.12.3.jar" -libraryjars "/Users/sbly/Library/Java/JavaVirtualMachines/corretto-11.0.12/Contents/Home" -outjars "/private/var/folders/34/jl47bvtd2_14y99v6gx4k0bm0000gt/T/sbt_d73d4659/target/scala-2.12/proguard/simple_2.12-0.1.0-SNAPSHOT.jar" -dontoptimize -dontnote -dontwarn -ignorewarnings -keep public class Test { [info] [info] public static void main(java.lang.String[]); [info] [info] } [info] [info] ProGuard, version 7.0.0 [info] [error] Error: The same input jar [/Users/sbly/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/scala-lang/scala-library/2.12.3/scala-library-2.12.3.jar] is specified twice. [info] [error] java.lang.RuntimeException: Proguard failed with exit code [1]

Depending on what other settings I tweak, I encounter other errors, but I can never get it working without assembly. If you can, that's great. Maybe my local setup is somehow broken

update: we removed sbt-assembly for the final version of this PR

sbly · 2021-08-18T15:11:03Z

@SethTisue Hey, is it possible to merge this, or did you want to hold off?

SethTisue · 2021-08-18T19:39:55Z

@mpollmeier do you have an opinion about the concern Peter raised about adding the dependency on sbt-assembly?

I have never used this plugin myself (though I used to be a heavy user of ProGuard itself, back in applet days 🙀), so I don't know how much my opinion is worth, but I share Peter's concern that it seems like a pretty big change, and we don't know if users are depending on the old behavior.

My understanding (correct me if i'm wrong) is that Proguard can only work on fat jars

I really doubt that's the case. At https://github.com/Guardsquare/proguard/blob/master/docs/md/manual/refcard.md we see -libraryjars .

pvlugter

With this building against sbt 1.5, it may not work with earlier sbt 1.x versions. Do we want to support all 1.x versions? Something we've done for other plugins is build against a 1.0 version, and rely on compatibility, with scripted tests for later sbt versions.

pvlugter · 2021-08-19T01:29:38Z

src/main/scala/com/typesafe/sbt/SbtProguard.scala

+    proguardBinaryDeps := {
+      (Compile / libraryDependencies).value match {
+        case analysis: Analysis =>
+          analysis.relations.allLibraryDeps.collect { case vf: PathBasedFile =>
+            vf.toPath.toFile
+          }.toSeq
+      }
+    },


Looks like this is no longer used.

Previously, the binary deps were pulled from the compile analysis to include all binaries, including Java's rt.jar. Looks like that's no longer included under library deps, and I see there's a java home added instead.

Would be good to make sure there's still something that works here across Java versions and environments.

Yeah, using this method the Java libraries were missing, which is why I added an explicit dependency on JAVA_HOME

src/main/scala/com/typesafe/sbt/SbtProguard.scala

SethTisue · 2021-08-19T02:31:13Z

With this building against sbt 1.5, it may not work with earlier sbt 1.x versions.

Ah, so, this is a subject I have opinions on... here they come :-)

Do we want to support all 1.x versions?

In short, no. People should upgrade to latest sbt if they want to use latest plugin versions. That's the decision I'm seeing most sbt plugin maintainers make.

I think it can definitely make sense in a commercial-support context to go the extra mile to ensure compatibility with old versions of things, but here in pure OSS-land where no money is changing hands (even though lots of people are making money using free stuff) and maintainer and contributor effort is the scarcest and most precious resource, I think the onus should be on users to put in a little effort and upgrade things if they want to benefit from the latest free OSS goodies — rather than the onus being on maintainers to accommodate version laggards. (I don't mean "laggards" as shade; people often have good reasons for being conservative about upgrades, but regardless, they should expect a decision not to upgrade to have consequences.)

Also, I'd add that the latest really disruptive change (though overall a hugely beneficial one) made to sbt was the Ivy->Coursier switch. That was a while ago now (sbt 1.4.0, May 2019). Once people are over the 1.4.0 hump — and they've now had plenty of time to get over it — it's only rarely a problem to go all the way to 1.5.5.

sbly · 2021-08-19T15:00:59Z

I am going to give it another shot without using assembly. Maybe I was just doing something wrong

sbly · 2021-08-19T20:18:10Z

I'm absolutely stuck on getting this to work. I can't figure it out. So we can close this PR if we don't want to bring in assembly and no one else wants to work on it

mpollmeier · 2021-08-24T06:43:08Z

My comment above #42 (comment) is easily missed in the noise, so linking here again.

I think we can just take out the sbt-assembly related additions and merge the remainder as is. The test projects in this repo (as well as our internal build) work just fine.

And separately from that we can try to help @sbly to solve his specific issue. If it turns out to be a genuine issue and we really do need sbt-assembly, it wouldn't be the end of the world either, but currently I don't see why we should.

mpollmeier · 2021-08-30T07:50:52Z

I forked this PR and pushed a few fixes. It's now not using sbt-assembly and filters out the duplicate input jars.
https://github.com/mpollmeier/sbt-proguard/tree/sbt-1.5

Could you please give that a try? If it works you could push those commits to this PR and we can merge it.

For reference: here's how I tested locally, not sure if that's the best way, but worked for me:

sbt 'clean; set version := "0.5.0+1-noassembly"; publishLocal'

cd src/sbt-test/proguard/<simple|merge|filter>
sbt -J-Dproject.version=0.5.0+1-noassembly proguard

sbly · 2021-08-31T21:05:02Z

Thanks for taking a stab at this. I cloned your repo and ran sbt scripted proguard/simple, which is how I've been testing. I got similar errors to what I had before. Does it work for you?

mpollmeier · 2021-09-01T08:13:30Z

I only just learned about sbt-test and made myself familiar (https://www.scala-sbt.org/1.x/docs/Testing-sbt-plugins.html).

And yes, I'm also getting an error there, but that's a different one now: Error: Invalid or corrupt jarfile - is that what you're getting?

It's because simple/test.sbt is running java -jar on the jar that proguard produces, and that doesn't have a MANIFEST.MF.
I didn't manage to configure proguard to keep the correct manifest unfortunately - do you have any ideas?

sbly · 2021-09-01T14:35:21Z

@mpollmeier OK cool, so it's not just my machine. I've gotten that specific error before too. I was able to get rid of it before by changing the default input filter:

proguardDefaultInputFilter := None

but then I just ran into another error.

mpollmeier · 2021-09-08T13:15:28Z

Ok I finally managed to sort this out, mostly by reverting some of the changes in this PR that were unrelated to the sbt upgrade.
Most importantly for fixing the MANIFEST issue above was the fact that you changed the proguard config for the simple test build.

I'm not saying that this is the best default value, but would suggest to focus this PR on purely upgrading sbt:
https://github.com/mpollmeier/sbt-proguard/tree/sbt-1.5

sbt scripted works with that.
@sbly Could you please give that a try? If it works you could push those commits to this PR and we can merge it.

Sbt 1.5

sbly · 2021-09-08T15:45:30Z

@mpollmeier Thank you so much. Do you know what the actual fix was? It seems to have just been a problem with the test itself

mpollmeier · 2021-09-08T18:05:22Z

The main differences to the initial PR were to remove sbt-assembly and get the test setup back to what it was - you removed some proguard settings in that simple test, which made it drop it's manifest.md.

@SethTisue @pvlugter 🙏🏻 I believe this is ready for merge/release - would you mind clicking a few buttons?

SethTisue

I've made one (admittedly a bit nitpicky) change request, otherwise LGTM for merge.

src/sbt-test/proguard/filter/build.sbt

filter specific scala version

SethTisue · 2021-09-09T19:31:13Z

thank you @sbly and @mpollmeier !!

ticket on actually getting this published: #43

sbly and others added 8 commits August 6, 2021 11:57

Upgrade to sbt 1.5.5; virtual file support

8ad8805

Simplify

35533e9

Bump major version

93862ab

Rewrite deprecated syntax

e22d7f7

Update .gitignore

ca9fd0a

Clean up a bit

700a55d

Make the plugin work with SBT 1.5

de6e984

Clean up

1a6d674

SethTisue closed this Aug 12, 2021

SethTisue reopened this Aug 12, 2021

No longer cross-build against sbt 0.13

ebc3dfa

pvlugter reviewed Aug 16, 2021

View reviewed changes

pvlugter reviewed Aug 19, 2021

View reviewed changes

remove sbt-assembly dependency

51c529f

mpollmeier added 4 commits August 29, 2021 19:37

gitignore

09e097e

upgrade scala version in test projects

2db376f

JAVA_HOME

08f270c

fix duplicate inputs

5cfbbad

mpollmeier added 3 commits September 1, 2021 09:54

update travis script

1d75ecb

can't get proguard to keep the right manifest - adapt test...

9f0ebee

back to old test

d0e6142

mpollmeier added 2 commits September 8, 2021 14:30

back to writing the proguard config file, to simplify debugging

78d3f2a

back to old config for test project

fd00768

sbly added 2 commits September 8, 2021 11:00

Merge pull request #1 from mpollmeier/sbt-1.5

64bc542

Sbt 1.5

Remove unused task key

9e34b05

SethTisue requested changes Sep 9, 2021

View reviewed changes

src/sbt-test/proguard/filter/build.sbt Outdated Show resolved Hide resolved

mpollmeier and others added 2 commits September 9, 2021 20:39

filter specific scala version

f3e562b

Merge pull request #2 from mpollmeier/sbt-1.5

3db76bd

filter specific scala version

This was referenced Sep 9, 2021

not working for sbt 1.4.6 #40

Closed

Publish new version #43

Closed

SethTisue approved these changes Sep 9, 2021

View reviewed changes

SethTisue merged commit 579dac3 into sbt:master Sep 9, 2021

Upgrade to SBT 1.5 #42

Upgrade to SBT 1.5 #42

Conversation

sbly commented Aug 11, 2021

SethTisue commented Aug 12, 2021

sbly commented Aug 12, 2021

SethTisue commented Aug 12, 2021 • edited Loading

SethTisue commented Aug 12, 2021 • edited Loading

sbly commented Aug 12, 2021 • edited Loading

SethTisue commented Aug 13, 2021

sbly commented Aug 13, 2021

SethTisue commented Aug 13, 2021 • edited Loading

mpollmeier commented Aug 14, 2021

pvlugter commented Aug 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbly commented Aug 18, 2021

SethTisue commented Aug 18, 2021 • edited Loading

pvlugter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SethTisue commented Aug 19, 2021 • edited Loading

sbly commented Aug 19, 2021

sbly commented Aug 19, 2021

mpollmeier commented Aug 24, 2021

mpollmeier commented Aug 30, 2021

sbly commented Aug 31, 2021

mpollmeier commented Sep 1, 2021

sbly commented Sep 1, 2021

mpollmeier commented Sep 8, 2021

sbly commented Sep 8, 2021

mpollmeier commented Sep 8, 2021

SethTisue left a comment

Choose a reason for hiding this comment

SethTisue commented Sep 9, 2021

SethTisue commented Aug 12, 2021 •

edited

Loading

SethTisue commented Aug 12, 2021 •

edited

Loading

sbly commented Aug 12, 2021 •

edited

Loading

SethTisue commented Aug 13, 2021 •

edited

Loading

SethTisue commented Aug 18, 2021 •

edited

Loading

SethTisue commented Aug 19, 2021 •

edited

Loading