WIP fix: Isolate codegen classpath from ModelAssembler#1705
WIP fix: Isolate codegen classpath from ModelAssembler#1705ghostbuster91 wants to merge 22 commits into
Conversation
b81b2be to
aa437f5
Compare
|
I appreciate and support an effort to isolate the smithy pipeline away from the program's main classloader, so long as you manage to get it to build :s |
This reverts commit 309c392.
SummaryIt is possible to achieve better isolation between the codegen's classpath and model validator's classpath. In the current approach the common denominator is smithy-model api that is shared between model validator classloader and the codegen's classloader. This has the consequence that 3rd party shapes that are present in both classloaders can be no longer queried by class, e.g. A class in JVM is identified by its FQN and the classloader that loaded it. Codegen protocolCodegen module used to define some transformations. Unless they are explicitly added into the bridging classloader they can't be seen by the model validator classloader. Instead I decided to extract them into a separate module that is passed together with the protocol module into the model assembly as external jar. OpenapiOne thing that I was not aware of before starting this work is that the codegen renders also openapi files using alloy-openapi module. This breaks once we put the stronger isolation in place because the smithy-openapi module compares traits by classes, not by id. One way to fix it is to actually rely on alloy-open being provided by user. If there is no such module provided than the codegen could simply skip openapi generation step. In order for arlib to use the provided classloader a small change is required alloy. I also explored the other approach where the codegen uses its own alloy-openapi to generate openapi files. This requires some changes in alloy and in smithy. Surely, the implementation might be more polished, I just wanted to get it working. The change itself might be desired but I am less in favor of this approach. |
| resolve: Boolean | ||
| ): Class[?] = { | ||
| if ( | ||
| name.startsWith("software.amazon.smithy") || name.startsWith("scala") |
There was a problem hiding this comment.
this looks fragile but maybe it won't have to be changed at all?
|
just sbt scripted tests are failing due to the use of alloy snapshot |
|
closing in favor of #1707 |
What problem does this solve?
This solves two problems.
First, there is an inconsistency in the current code regarding how the shapes are loaded vs how classes are loaded.
We look for shapes manually via:
This scans only external jars. It will not discover shapes on the current classpath.
However, the
validatorClassLoaderthat is passed toModelAssembleris constructed as:Which means that SPI will load matching implementation also from the current classpath, because the
currentClassLoaderis the parent of thevalidatorClassLoader.So it might be possible that a java part of the shape, such as
TraitService$Provider, is found but its Smithy counterpart is not. This might be confusing.Second, because
currentClassLoaderis the parent ofvalidatorClassLoaderany class that is on thecurrentClassLoaderclasspath will be loaded by that classloader. This can lead to a very surprising behaviors if a class is changed between the versions of a jar that belongs to both classloaders.Example:
Smithy4sCodegen depends on
protocolmodule, so it has it on its classpath.protocolmodule definesAdtMemberTraitValidator.Now, if you try to generate code for a smithy artifact that depends on another version of the protocol that has a different version of
AdtMemberTraitValidatoryou won't see that version but instead you will see the one from the codegen classpath because of the parent-first delegation rule.This PR currently fails to build. I suppose that it is due to some missing transformations from the current classpath that need to be somehow added into the isolated loader.
An alternative approach to this would be to create a proxy using reflection and have a full isolation but I believe that it is too cumbersome.
PR Checklist (not all items are relevant to all PRs)