Run built-in plugins before others

[gosar]

When smithy build fails on a plugin that is not built-in (i.e., not model, build-info or sources), even though I think the model itself is good, depending on the order of plugins, the model plugin may not have been run yet. Having the model plugin run before would allow seeing the full model serialized even when other plugins fail.

I found that the plugins end up in a TreeMap here making it natural order. It uses LinkedHashMap in the SmithyBuildConfig here, so it seems if we use LinkedHashMap instead of TreeMap in SmithyBuildImpl, we could make sure the BUILTIN_PLUGINS always get run first.

[milesziemer]

It looks like BUILTIN_PLUGINS are added right before SmithyBuildConfig is built here, so the built-in plugins are always after any other plugins present in SmithyBuildConfig. I think we would need a combination of using LinkedHashMap in SmithyBuildImpl, and some way to make sure that BUILTIN_PLUGINS come before any other plugins when SmithyBuildConfig is built.

As this comment says, we can't add BUILTIN_PLUGINS before this point because they could be removed when plugins() is called. Plugins can also be set when a config file is loaded.

I guess a solution could be to just use a temporary Map to reverse the ordering.

[mtdowling]

Would it be possible to instead not fail the whole build if one plugin fails, and defer the failure until the end?

[milesziemer]

I've been looking into deferring the failure until the end a bit more. When a plugin fails, the error is caught here in SmithyBuildImpl, and is passed on to the exception consumer. The plugins are applied in a loop when running the projection, so we could catch and aggregate any errors within that loop to allow the rest of the plugins to run. The projection still needs to fail, so I think a solution would be to use the individual plugin errors to create one big projection error, then throw that after all the plugins have run.

I think a better solution might involve some refactoring of how SmithyBuildImpl works, as it is a bit messy, especially if we intend on changing how projections run (see this comment). I couldn't think of any obvious way to refactor it, but for now I don't see this change adding too much complexity so I can get a PR up for it if that sounds good, unless I'm missing something.

[kstich]

This was resolved in #1762 , running all plugins for a projection before failure.