HHH-20256 Make ByteBuddy bytecode generation reproducible#11982
HHH-20256 Make ByteBuddy bytecode generation reproducible#11982gsmet wants to merge 1 commit intohibernate:mainfrom
Conversation
|
Actually, I found another issue regarding the order in which the methods are implemented, I need to see how we can fix that. |
|
I created an issue in ByteBuddy for the additional issues I spotted as I think the fixes should live there: raphw/byte-buddy#1890 . |
gsmet
force-pushed
the
fix-reproducibility
branch
from
90305d0 to
e69ef5c
Compare
| byteBuddy = new ByteBuddy( classFileVersion ) | ||
| .with( TypeValidation.DISABLED ) | ||
| // we use a fixed suffix to make sure our builds are reproducible | ||
| // ByteBuddy uses a random suffix by default to be extremely safe | ||
| // in case the class is instrumented multiple times | ||
| // but in our case, we can use a common suffix affected to Hibernate ORM | ||
| .with( new Implementation.Context.Default.Factory.WithFixedSuffix( "hibernate" ) ); |
There was a problem hiding this comment.
There was a problem hiding this comment.
if i remember well the issue was discussed here #hibernate-orm-dev > ✔ Seeing EE TCK ClassCastException with ORM 6.6.13.Final
There was a problem hiding this comment.
Yeah so from what I have seen the original PR was: https://github.com/hibernate/hibernate-orm/pull/9848/changes .
There was a problem hiding this comment.
AFAICS, we apply a fixed name in every non-deprecated code path. I see that org.hibernate.proxy.pojo.bytebuddy.ByteBuddyProxyHelper#buildUnloadedProxy(net.bytebuddy.pool.TypePool, net.bytebuddy.description.type.TypeDefinition, java.util.Collection<? extends net.bytebuddy.description.type.TypeDefinition>) has a comment, that it is used by Quarkus, but even that uses a fixed name.
Can you share the code paths that produce random names so I can better understand what is going on? Chances are that Quarkus is just using deprecated methods and should switch to the newer methods now.
There was a problem hiding this comment.
From the pull request, it looks like the change they did was a lot more involved that what I'm trying to achieve here.
If I'm not mistaken tests were added after the report and all tests are green with this PR.
There was a problem hiding this comment.
Can you share the code paths that produce random names so I can better understand what is going on? Chances are that Quarkus is just using deprecated methods and should switch to the newer methods now.
Not sure about the code paths, but the issue description shows the symptoms:
I think this is a different problem then: @beikov made sure the name of proxy classes is stable, while @gsmet is making sure the name of fields within proxy classes is stable.
If I'm not mistaken tests were added after the report and all tests are green with this PR.
Good. Then if you're certain this "fixed suffix" thing will only affect proxy internals, and we won't end up with unwanted suffixes in e.g. bytecode-enhanced accessors (the generated $$_hibernate_read_myProperty() methods)... I suppose it's safe?
It would feel slightly safer to just add this .with( new Implementation.Context.Default.Factory.WithFixedSuffix( "hibernate" ) ) inside org.hibernate.proxy.pojo.bytebuddy.ByteBuddyProxyHelper#proxyBuilder, but maybe I'm worrying too much.
There was a problem hiding this comment.
Ok, after looking into the ByteBuddy code, it seems that this name only affects cached accessors and field values, which was not clear to me when looking at this. Might be worth spelling this out in the comment. Can you please make that clarification @gsmet?
By using a fixed suffix for everything generated by Hibernate ORM. This will allow for reproducible builds. Note that the resulting name still has an additional suffix which is related to the current element. Usually, it's a hashCode representing the element, hashCode that is stable from builds to builds.
gsmet
force-pushed
the
fix-reproducibility
branch
from
e69ef5c to
82f1000
Compare
4 participants
By using a fixed suffix for everything generated by Hibernate ORM. This will allow for reproducible builds.
Note that the resulting name still has an additional suffix which is related to the current element. Usually, it's a hashCode representing the element, hashCode that is stable from builds to builds.
@beikov @mbellade @yrodiere if we decide to go with this one, I would very much appreciate if we could backport it to the version that will land in upcoming Quarkus. If it is deemed safe enough, of course.
https://hibernate.atlassian.net/browse/HHH-20256
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.
Please make sure that the following tasks are completed:
Tasks specific to HHH-20256 (Improvement):
documentation/src/main/asciidoc/userguidefor all features,documentation/src/main/asciidoc/introductionfor main features, links from existing documentationmigration-guide.adoc(breaking changes) andwhats-new.adoc(new features/improvements)