https://pantsbuild.org/ logo
#development
Title
# development
s

salmon-summer-38098

10/24/2018, 4:48 PM
that way when there are no sources, in that codepath, we could explicitly specify a
pom
packaging
e

enough-analyst-54434

10/24/2018, 4:49 PM
Yeah - I'm more concerned with enabling aggregator poms at all. There are 0 good use cases for them. I think my latest comment on the PR explains why an aggregator is always bad. We should not add affordances to create them imo.
This is particularly striking for util / finagle since the suffering from the netty rename was felt acutely! I think though that was either far enough in the past or else csl is not connecting the dots that they are doing the same thing all over again.
I'm happy to explain the problem better if its unclear. It probably deserves a doc or blogpost or PR to semver.org
👍 1
s

salmon-summer-38098

10/24/2018, 4:54 PM
I think its mostly clear but to state the gist that I understand - when they went from one one project to several smaller projects - they should have renamed the top level?
w

witty-crayon-22786

10/24/2018, 4:55 PM
@enough-analyst-54434: i did describe a usecase in my reply on the issue
e

enough-analyst-54434

10/24/2018, 4:55 PM
the should have renamed classes.
The use case is just not valid
w

witty-crayon-22786

10/24/2018, 4:55 PM
while i agree that "breaking your api" is a solution, it's an extremely expensive solution
e

enough-analyst-54434

10/24/2018, 4:56 PM
Its the only right one
Its just a not well understood fact that packaging a public API - in any language - is a heavy choice
Devs understand the public API itself is a heavy choice
But they haven't internalized that picking the packaging is too
w

witty-crayon-22786

10/24/2018, 4:57 PM
well, sure. but there are heavy choices, and then there are hairshirts
e

enough-analyst-54434

10/24/2018, 4:57 PM
over my head
w

witty-crayon-22786

10/24/2018, 4:57 PM
monks would wear "hairshirts" to punish themselves
e

enough-analyst-54434

10/24/2018, 4:57 PM
Ah, never heard of that
At any case, this looks to me like the user can already workaround. And the pain pants provides sends all the right signals
w

witty-crayon-22786

10/24/2018, 4:58 PM
(i'm probably misspelling it)
e

enough-analyst-54434

10/24/2018, 4:58 PM
SO this feels like caving to bad behavior - unintentional - but bad behavior
w

witty-crayon-22786

10/24/2018, 4:58 PM
i'm not sure i agree. telling people that it should be painful to refactor their code is not a great signal
our goal should be to enable that
7 years after util-core was created they finally decided to follow go/111
e

enough-analyst-54434

10/24/2018, 4:59 PM
That I agree, but agrregator poms don't do that
w

witty-crayon-22786

10/24/2018, 4:59 PM
i would love to not make that painful for them
e

enough-analyst-54434

10/24/2018, 4:59 PM
shading magic, custom classloaders, etc do
w

witty-crayon-22786

10/24/2018, 4:59 PM
it enables a more seemless migration, as mentioned above
e

enough-analyst-54434

10/24/2018, 4:59 PM
Alright - we fundamentally disagree on this one. It enable lazy migration, not seamless
At any rate, the pain existing here for the user is non-existent, an ugly classfile. The ugly classfile could be renamed - BeCarefuleNextTimeBecauseTheecosystemDoesNotYetSupportJarRefactoringEvenThoughtPantsMakesItLookEasy
I do agree, a comprehensive solution that actually supported jar or python dist or what have you re-granularization would be great.
7 years after util-core was created they finally decided to follow go/111
That is great! The problem was not continuing to publish the original fatter jar until sane migration could be figured out for folks who needed jars.
w

witty-crayon-22786

10/24/2018, 5:11 PM
hm. the ecosystem supports
pom
packaging, no?
e

enough-analyst-54434

10/24/2018, 5:11 PM
It does, but it suffers the problem you pointed out. In any ecosystem doing resolving of this sort, an aggregator dep set subverts the resolver
w

witty-crayon-22786

10/24/2018, 5:12 PM
hm... wait. i was suggesting that an aggregator is a solution to the two problems i showed
e

enough-analyst-54434

10/24/2018, 5:12 PM
you can get same name via different artifacts and runtime issue
It is not if I've thought through this right
w

witty-crayon-22786

10/24/2018, 5:12 PM
if the aggregator exists, it solves this problem
e

enough-analyst-54434

10/24/2018, 5:12 PM
Let me sketch a bit, I believe it definitely does not
w

witty-crayon-22786

10/24/2018, 5:13 PM
two distinct subgraphs are not joined unless they share an artifact id
without the aggregator, they are not joined
e

enough-analyst-54434

10/24/2018, 5:14 PM
Just so I'm clear on what util did, they took an artifact named bob that contained a bunch of classes and turned bob into an emptry aggregator that pointed at jane and greg, each of which got a portion of the original classes owned by bob
Is that right?
s

salmon-summer-38098

10/24/2018, 5:16 PM
thats correct
e

enough-analyst-54434

10/24/2018, 5:17 PM
OK - sketching...
w

witty-crayon-22786

10/24/2018, 5:17 PM
@enough-analyst-54434, @salmon-summer-38098: https://photos.app.goo.gl/AYWVK3KPgRD8sfHi9
with no aggregator, there is nothing to "knock out"
util-core#old
, so you get two copies of classfiles
with an aggregator,
util-core#new
(is pom-packaged and) knocks out
util-core#old
e

enough-analyst-54434

10/24/2018, 5:21 PM
Copy code
T:
bob.1 [a.1, b.1]

T+1:
bob.2
  -> jane.1 [a.1]
  -> greg.1 [b.2]

project
  -> greg.1
  -> foo -> bob.1
Here there is T(old util) and T+1(new util)
Since util did not rename classes, project blows up at runtime since both file b.1 and b.2 are on the classpath
And b.1 and b.2 are the same filename, just API changed versions of the file.
w

witty-crayon-22786

10/24/2018, 5:22 PM
(responding to borja's thing, back in a sec)
e

enough-analyst-54434

10/24/2018, 5:24 PM
Scanning back through what you said Stu - the knockout case is the happy path. I illustrate the path when there is no aggregator-level knock-out.
Basically, any time you have a dependency on the old fat artifact and transitively you get a library doing the right thing and depending directly on the new thinner targets.
I'll finish off an issue I have started tracking enabling the hard case the right way. Again - I do agree enabling artifact refactoring would be amazing. But an aggregator artifact just doesn't do it.
w

witty-crayon-22786

10/24/2018, 5:30 PM
@enough-analyst-54434: i don't understand why...
in the diagram i showed, are you suggesting that
new
does not knock out
old
?
e

enough-analyst-54434

10/24/2018, 5:31 PM
Knock-out totally works. It does not cover all cases.
w

witty-crayon-22786

10/24/2018, 5:31 PM
(keeping in mind that this would likely be transitively somewhere under that
dep
root i drew)
hm, ok
in the sense that once you remove the aggregator, the solution stops working?
e

enough-analyst-54434

10/24/2018, 5:32 PM
All I'm saying is an aggregator does not solve the problem of moving artifact contents or splitting them. My example shows how it fails.
It solves the happy path, so may be better than not having one. But currently the only way to move or split artifacts safely in jvm, python, anything I'm aware of - is renaming symbols
w

witty-crayon-22786

10/24/2018, 5:33 PM
ahhh, i see. if someone has already migrated to the new split packages, then they lose the protection of the aggregator
e

enough-analyst-54434

10/24/2018, 5:34 PM
Solving that issue well is exactly the sort of thing Toolchain would like to do
Yes
w

witty-crayon-22786

10/24/2018, 5:34 PM
ok, thanks
e

enough-analyst-54434

10/24/2018, 5:35 PM
Yeah, hopefully util understands the devil's bargain they made. Users will hit problems even with an aggregator.
The only right way to do this in today's world is to be up front and rename symbols and let the user suffer compile time pain
w

witty-crayon-22786

10/24/2018, 5:36 PM
given that it is a (even flawed) tool that the ecosystem provides though, exposing the tool seems worthwhile
e

enough-analyst-54434

10/24/2018, 5:36 PM
That is what I disagree on
Anyone who wants to make the arguably poor choice can do it
They just need to hack in a file
w

witty-crayon-22786

10/24/2018, 5:36 PM
... but i see what you're saying. if people think that it works and is enough, then they'll get a false sense of security.
e

enough-analyst-54434

10/24/2018, 5:36 PM
I think that is great peer pressure from Pants not to do it or at least to realize what is being done is shady
The probelm is that the peer pressure is not direct and explicit, so in the issue I filed an 'a' option
Which is to make whatever Ity's PR lands on a more explicity called out problem maybe with warnings? I don't know.
.. but i see what you're saying. if people think that it works and is enough, then they'll get a false sense of security.
FWIW - this was exactly the case with the netty rename debacle
s

salmon-summer-38098

10/24/2018, 5:50 PM
reading both of yours explanation and thinking about this more - I think it makes sense to discard this PR.
I think making aggregator pom an easier solution will end up hiding the fact that this is not an ideal way to refactor.
although I agree, we need a written explanation for this somewhere
👍 1
w

witty-crayon-22786

10/24/2018, 5:50 PM
yep, agreed. thanks for the discussion john.
s

salmon-summer-38098

10/24/2018, 5:50 PM
since, its something thats bound to come up again (and it sounds like it had come up at some point in time earlier)
e

enough-analyst-54434

10/24/2018, 5:51 PM
OK - great. Thanks for bearing with me on the round-about explanation.
s

salmon-summer-38098

10/24/2018, 5:51 PM
no, thank you for taking the time to explain!
both of you 🙂
e

enough-analyst-54434

10/24/2018, 6:08 PM
I transferred the bob jane & greg explanation to the issue and expanded a bit. Hopefully clear and maybe something you can point complaining folks to if they are unhappy.