Establish the codegen-core module

[jdisanti]

Motivation and Context

This PR establishes a codegen-core module so that pieces of the old codegen module that codegen-server has been reusing can opportunistically be refactored out so that there is a better separation of the client/server code generation logic. The high level changes are:

• Create the codegen-core module
• Add codegen-core as a dependency to codegen and codegen-server
• Move the Version class into codegen-core so that it has at least one thing in it
• Rename codegen to codegen-client
• Rename codegen-test to codegen-client-test

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[david-perez]

Thank you!

I wonder, should we place the Smithy models the integration tests use in a common place?

Currently all of the server ones except pokemon.smithy and simple.smithy are symlinks to the actual models that (now) live under codegen-client-test/model.

Analogously, the client ones are all actual files except for pokemon.smithy and simple.smithy, which are symlinks.

[jdisanti]

I wonder, should we place the Smithy models the integration tests use in a common place?

I like this idea. Looks like I need to fix the integration tests anyway, so I'll take a stab at it.

[jdisanti]

While moving the shared models to a central location, I discovered a few validation errors in misc.smithy. I'm actually not sure how this ever worked since Smithy refuses to load this model with these errors, but I guess it didn't run validation for non-imports before? I applied some fixes to this model that I don't think should impact the tests at all, but this deserves some additional scrutiny.

[david-perez]

I'm actually not sure how this ever worked since Smithy refuses to load this model with these errors

That's very interesting. I can indeed reproduce that on upstream/main, no errors are yielded for misc.smithy, but now that we're using imports = in our CodegenTests, we get them without 6c74dbf applied. Why can't we load the common models from the classpath (like we do for the protocol tests on awslabs/smithy), instead of using imports?

---

I guess it didn't run validation for non-imports before

That would be my guess too. Is that intentional on Smithy's behalf? That behavior is unexpected and should be documented if so. But I suspect it's a bug and we should cut them an issue.

---

but this deserves some additional scrutiny.

The changes in 6c74dbf look good to me. However, is failing build on DANGER events appropriate for http.code for a 4xx code? The spec says:

The provided value SHOULD be between 100 and 599, and it MUST be between 100 and 999.

I would expect the build to fail were it a MUST, but it's a SHOULD in this case.

[david-perez]

Suggested change

	"\"imports\": [${imports.map { "\"$it\"" }.joinToString(", ")}],"
	"\"imports\": [${imports.joinToString(", ") { "\"$it\"" }}],"

[jdisanti]

Why can't we load the common models from the classpath (like we do for the protocol tests on awslabs/smithy), instead of using imports?

I think it would be a bad idea to have test models in the codegen-core.jar file, since those will inevitably get loaded up when a customer goes to generate a client or a server. We can have them in the classpath for codegen-client-test and codegen-server-test since those aren't libraries. It seems to me like we should even go in the other direction and import everything rather than use classpaths so that we get validation.

I suppose we could also have a separate module entirely just for the common models, as an alternative. But I think getting validation via imports is probably better.

Edit: Going to hold off on making any changes here. I think this needs to be addressed in Smithy.

[jdisanti]

However, is failing build on DANGER events appropriate for http.code for a 4xx code?

Something seems wrong here. Not sure if it's the spec or the error. I filed smithy-lang/smithy#1386 on Smithy.

[jdisanti]

Filed smithy-lang/smithy#1387 on Smithy to track the validation oddity. Also added my findings to #1489.

[jdisanti]

The PR bot failure to create a diff is expected since the script is now referencing codegen-core. Generating the previous revision's code to diff against will always fail until this is merged into main.

[david-perez]

I think this thing of loading using imports is better in terms of speed too!

With this patch, if I mess up e.g. misc.smithy and do:

./gradlew codegen-client-test:build -P modules='simple' -P cargoCommands='test'

Then the build succeeds. Without this patch, currently on origin/main, the build fails saying that misc.smithy is not valid. So I think that now validation is only happening for the model you actually generate, which should make development faster!

[david-perez]

Indeed, with this patch applied, if I set a breakpoint in the codegen visitor and inspect the model, I see that only shapes coming from the model(s) I'm generating and from the awslabs/smithy repository are being loaded, whereas without this patch, the entirety of the models listed in codegen-test/build.gradle.kts and the ones from the awslabs/smithy repository are being loaded, regardless of the model(s) I'm generating.

It'd be nice if the models vended by awslabs/smithy were not bundled in the JAR then, which I presume is the reason why they are always being loaded and validated even when you're not generating them. That would speed things up substantially, since they define the bulk of the shapes being loaded (more than 2000).