Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a dependency tracking mechanism #607

Merged
merged 10 commits into from
Nov 23, 2022
Merged

Conversation

Baccata
Copy link
Contributor

@Baccata Baccata commented Nov 16, 2022

This facilitates the tracking of dependencies without imposing additional burden on the user to know what dependencies were used to generate code in upstream libraries/projects.

Essentially, what it does is add a line in the manifest of project/modules that have the Smithy4s plugin enabled, tracking dependencies locally declared with % Smithys. A bit of logic is added so that downstream projects that also have Smithy4s enabled automatically resolve these dependencies before running code-generation.

Additionally, this PR changes the build plugins so that libraryDependencies are taken into account for codegen :

Now, by default, any library declared as a "normal" dependency, by means of ivyDeps in mill or libraryDependencies in sbt are used during the code-generation process. This makes Smithy4s more consistent with itself, as the same behaviour is applied to internal dependencies. It is also argueably more intuitive to do it this way.

Todo

  • Add the same mechanism in the Mill integration

  • Dogfooding : make sure that the artifacts published by this build (which do not use the Smithy4sCodegenPlugin, cause it's also published by this build) contain the correct line in the manifest, so that users don't have to add explicitly to their builds dependencies to protocol-test-traits, aws-traits, etc ...

This facilitates the tracking of dependencies without imposing additional
burden on the user to know what dependencies were used to generate code
in upstream libraries/projects
}

private lazy val simple = raw"([^:]*):([^:]*):([^:]*)".r
private lazy val cross = raw"([^:]*)::([^:]*):([^:]*)".r
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just spitballing, could we use coursier's api to parse the dependency strings?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and then how do you go from coursier dep to SBT dep ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dunno, if it was a structured parsing result we could probably do that - if there's no way then the answer to my question is "no" ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's postpone this : right now the problem is more about "can we you render a dependency as a coursier-parsable string in both SBT and mill ?" Because I couldn't find a way to do that easily, I have the control over the format and parsing with coursier is not necessary as we're falling in the simplest possible cases (normal java dep, normal scala dep).

If it proves to be limitative we'll revise.

@kubukoz
Copy link
Member

kubukoz commented Nov 17, 2022

good stuff

and publish this project to an artifact repository, the Jar manifest will contain the following line :

```
smithy4sDependencies: software.amazon.smithy:smithy-aws-iam-traits:1.14.1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may be worth noting that multiple dependencies are comma separated

The same smithy4sDependencies metadata is now being populated for the
artifacts published in the smithy4s repository
@Baccata Baccata marked this pull request as ready for review November 21, 2022 17:50
import org.portablescala.sbtplatformdeps.PlatformDepsPlugin.autoImport._
import Smithy4sBuildPlugin.autoImport.isCE3

object Dependencies {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved this from build.sbt to be able to re-use in Smithy4sBuildPlugin

@@ -174,7 +299,7 @@ object Smithy4sCodegenPlugin extends AutoPlugin {
output = os.Path(outputPath),
resourceOutput = os.Path(resourceOutputPath),
skip = skipSet,
discoverModels = true, // we need protocol here
discoverModels = false, // we need protocol here
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
discoverModels = false, // we need protocol here
discoverModels = false,

I guess that comment can be removed now

modules/docs/markdown/01-overview/05-sharing-specs.md Outdated Show resolved Hide resolved
@kubukoz
Copy link
Member

kubukoz commented Nov 22, 2022

something is not yes (this test wasn't running earlier because there was an .only in the spec):

image

…ccount for codegen

Now, by default, any library declared as a "normal" dependency, by means
of `ivyDeps` in mill or `libraryDependencies` in sbt are used during the
code-generation process.

This makes Smithy4s more consistent with itself, as the same behaviour
is applied to internal dependencies.

It is also argueably more intuitive to do it this way.
Copy link
Contributor

@lewisjkl lewisjkl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I like this a lot more, much more clear to me. Thanks!

@Baccata Baccata merged commit 03ab174 into series/0.17 Nov 23, 2022
@Baccata Baccata deleted the dependency-tracking branch November 23, 2022 09:47
@Baccata Baccata added this to the 0.17.0 milestone Nov 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants