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

Use avro scope for external avro sources #211

Merged
merged 3 commits into from
Nov 8, 2024
Merged

Conversation

RustedBones
Copy link
Collaborator

@RustedBones RustedBones commented Nov 6, 2024

Rely on the Avro scope to compile external sources by default.

When avro sources are declared as Compile or Test dependencies, those would be fetched when depending on the default artifact (no avro classifier). The default artifact usually contains the java generated classes, hence the the plugin should not automatically recompile those.

Rely on the Avro scope to compile external sources.

If avro sources are declared as Compile or Test dependencies,
the plugin would automatically recompile sources, leading to duplicated
or even conflicting classes.
Comment on lines 10 to 12
{
"name": "referencedTypeField",
"type": "com.github.sbt.avro.test.external.Avsc"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To support such things, we would need to re-compile the schemas from the compile scope

Copy link
Contributor

@kellen kellen Nov 6, 2024

Choose a reason for hiding this comment

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

So in this case an external schemas lib can have no deps on other schema libs?

Or do we the end-user "just" need to include both external libs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we can (it is done in the Compile scope).
here we want the schemas in the test scope to depend on some other schema from the compile. To do so, we need to parse those again.

@@ -52,12 +52,14 @@ lazy val root: Project = project
name := "publishing-test",
crossScalaVersions := Seq("2.13.15", "2.12.20"),
libraryDependencies ++= Seq(
"com.github.sbt" % "transitive" % "0.0.1-SNAPSHOT" classifier "avro",
"com.github.sbt" % "transitive" % "0.0.1-SNAPSHOT" % Test classifier "tests",
"com.github.sbt" % "external" % "0.0.1-SNAPSHOT" % "avro" classifier "avro", // must be explicit
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

with avro scope, libs are not fetched transitively.

@@ -103,15 +103,23 @@ Compile / packageAvro / publishArtifact := true
You can specify a dependency on an avro source artifact that contains the schemas like so:

```sbt
libraryDependencies += "org" % "name" % "rev" classifier "avro"
libraryDependencies += "org" % "name" % "rev" % "avro" classifier "avro"
Copy link
Contributor

Choose a reason for hiding this comment

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

If people use the previous dependency declaration will the transitively included deps be compiled into the current artifact?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

there are 2 cases when user update to sbt-avro v4 :

  • if they require the schemas to be compiled: compilation fails unless user changes the dep scope to avro (or update the include filter)
  • if one lib pulls an undesired avro classifier artifact, the plugin won't re-compile the schemas (we assume the jar pulling the schema already includes compiled classes)

@RustedBones RustedBones merged commit 9bf76f8 into main Nov 8, 2024
4 checks passed
@RustedBones RustedBones deleted the avro-dependencies-scope branch November 8, 2024 08:23
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.

2 participants