-
Notifications
You must be signed in to change notification settings - Fork 17
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
Inline pprint to avoid binary incompatibilities #154
Conversation
d1ea144
to
7d27d26
Compare
import language.experimental.macros | ||
import reflect.macros.blackbox.Context | ||
|
||
trait TPrintLowPri { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only inlined TPrint part, but maybe we should inline it all? This way we could drop the dependency on pprint in mdoc too and this has been causing issues.
7d27d26
to
e10b279
Compare
e10b279
to
282edf0
Compare
a simple question: why not simply fix the pprint version in dependencies and never change it? |
or shade it, so others do not see it and are not affected by it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inlining sounds good
Metaconfig is an upstream dependency for multiple projects making it difficult to pin the version because downstream users can override the pprint version.
That solution would work but shading is generally painful (complicates the builds, slows down compilation, difficult to reason about compile/runtime behavior). Shading is justified in some cases, but for Metaconfig I think inlining is a better choice because we want to freeze pprint (ignore future updates). |
lazy val core = crossProject(JVMPlatform, JSPlatform, NativePlatform) | ||
.in(file("metaconfig-core")) | ||
.settings( | ||
sharedSettings, | ||
moduleName := "metaconfig-core", | ||
libraryDependencies ++= List( | ||
"org.typelevel" %%% "paiges-core" % "0.4.2", | ||
"org.scala-lang.modules" %%% "scala-collection-compat" % "2.5.0", | ||
"com.lihaoyi" %%% "pprint" % "0.7.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To address scalacenter/scalafix#1522 (comment), a version prior to com-lihaoyi/PPrint#72 should remain pinned in the classpath so that code compiled against old versions of metaconfig (<=0.9.15) can still run with upcoming versions (as a macro generates code that relies on it). If we do that code compiled against 0.9.16 will start breaking but considering it's very recent, I'd say it's acceptable if we tag a 0.9.17 soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is the example with https://github.com/NeQuissimus/sort-imports/blob/4e85753a52756106f5e21c2a23b721413f2571c3/rules/src/main/scala/fix/SortImports.scala#L19-L21 to illustrate:
$ javap -p -c -cp /tmp/sort-imports_2.13-0.5.4.jar fix.SortImportsConfig\$ | grep pprint
42: getstatic #66 // Field pprint/TPrint$.MODULE$:Lpprint/TPrint$;
50: invokevirtual #91 // Method pprint/TPrint$.lambda:(Lscala/Function1;)Lpprint/TPrint;
53: getstatic #94 // Field pprint/TPrintColors$BlackWhite$.MODULE$:Lpprint/TPrintColors$BlackWhite$;
56: invokeinterface #99, 2 // InterfaceMethod pprint/TPrint.render:(Lpprint/TPrintColors;)Ljava/lang/String;
97: getstatic #66 // Field pprint/TPrint$.MODULE$:Lpprint/TPrint$;
105: invokevirtual #91 // Method pprint/TPrint$.lambda:(Lscala/Function1;)Lpprint/TPrint;
108: getstatic #94 // Field pprint/TPrintColors$BlackWhite$.MODULE$:Lpprint/TPrintColors$BlackWhite$;
111: invokeinterface #99, 2 // InterfaceMethod pprint/TPrint.render:(Lpprint/TPrintColors;)Ljava/lang/String;
public static final java.lang.String $anonfun$surface$2(pprint.TPrintColors);
10: invokevirtual #195 // Method pprint/TPrintColors.typeColor:()Lfansi/Attrs;
public static final java.lang.String $anonfun$surface$1(pprint.TPrintColors);
10: invokevirtual #195 // Method pprint/TPrintColors.typeColor:()Lfansi/Attrs;
37: getstatic #66 // Field pprint/TPrint$.MODULE$:Lpprint/TPrint$;
40: getstatic #66 // Field pprint/TPrint$.MODULE$:Lpprint/TPrint$;
48: invokevirtual #91 // Method pprint/TPrint$.lambda:(Lscala/Function1;)Lpprint/TPrint;
51: invokevirtual #238 // Method pprint/TPrint$.implicitly:(Lpprint/TPrint;)Lpprint/TPrint;
55: invokeinterface #99, 2 // InterfaceMethod pprint/TPrint.render:(Lpprint/TPrintColors;)Ljava/lang/String;
public static final java.lang.String $anonfun$surface$3(pprint.TPrintColors);
10: invokevirtual #195 // Method pprint/TPrintColors.typeColor:()Lfansi/Attrs;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To address scalacenter/scalafix#1522 (comment), a version prior to com-lihaoyi/PPrint#72 should remain pinned in the classpath
I am afraid there is no actual way to do that.
I inlined pprint as metaconfig.pprint
to avoid any issues in the future of conflicting versions, not sure this will help in this situation then, but it's something we need to do. We can't inline pprint as just pprint
since that doesn't solve the issue when pprint would be on the classpath.
Is there any way actually to migrate to newer/inlined pprint and at the same allowing scalafix compiled rules to still work?
Or keeping a separate 0.9.x branch is the only way? I don't think we would be able to keep a separate branch really :/
Will it work if older pprint is kept on the classpath of scalafix? Since the generated macros would use that and the metaconfig.print
classes would not clash? To be honest I am a bit lost and I am afraid that there is nothing we can do aside from recompiling all those rules.
Since, there is not that much happening on metaconfig side I would say it's safe to just keep at the current version of metaconfig until we bump scalafix version to an incompatible one. What do you think? I am totally open to suggestions, the pprint incompatibility cause a whole lot of unexpected problems 😓
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coming to think of it maybe Since the generated macros would use that and the metaconfig.print classes would not clash?
would work? Since there is no actual clash between the current metaconfig pprint and the old one? Are we able to test that out with the current snapshot?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coming to think of it maybe
Since the generated macros would use that and the metaconfig.print classes would not clash?
would work? Since there is no actual clash between the current metaconfig pprint and the old one?
That's my understanding/suggestion yes - leaving the real pprint in the dependencies of metaconfig for a while (even if effectively not used at compile time), just to satisfy linking when used with client code compiled in the past.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we instead add the old pprint as a dependency to scalafix? Or would be better to keep it in metaconfig?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we instead add the old pprint as a dependency to scalafix? Or would be better to keep it in metaconfig?
That would work indeed 👍 If scalafix is the only user of metaconfig really affected by the breaking change introduced in metaconfig 0.9.16, I am happy to do that!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be only a scalafix problem as far I know, since metaconfig is transitive dependency for those rules. I will do a next release though 0.10.0
just in case since effecitively we change the binary compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will do a next release though
0.10.0
just in case since effecitively we change the binary compatibility.
👍 for a minor bump over a patch one. 0.9.16 was breaking too though, but I guess tagging a 0.9.17 is a hassle. In any case, fine for me, thanks for looking into this!
No description provided.