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

Migrate from sbt-protobuf to sbt-protoc #4483

Merged
merged 4 commits into from Aug 26, 2022
Merged

Migrate from sbt-protobuf to sbt-protoc #4483

merged 4 commits into from Aug 26, 2022

Conversation

RustedBones
Copy link
Contributor

This enables grpc java codegen support,

Clean useless scio-schemas. Move test schemas to project's Test configuration.

@codecov
Copy link

codecov bot commented Aug 1, 2022

Codecov Report

Merging #4483 (1171aaa) into main (f2d69f9) will decrease coverage by 0.11%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #4483      +/-   ##
==========================================
- Coverage   60.26%   60.14%   -0.12%     
==========================================
  Files         275      274       -1     
  Lines       10046    10014      -32     
  Branches      839      839              
==========================================
- Hits         6054     6023      -31     
+ Misses       3992     3991       -1     
Impacted Files Coverage Δ
.../com/spotify/scio/parquet/read/ParquetReadFn.scala 84.90% <0.00%> (-0.95%) ⬇️
...c/test/scala/com/spotify/scio/avro/AvroUtils.scala
...y/scio/values/PairSkewedSCollectionFunctions.scala 95.94% <0.00%> (+2.70%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Contributor

@clairemcginty clairemcginty left a comment

Choose a reason for hiding this comment

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

I think there may be some issue with sbt-protoc and m1 mac support ... I checked out this branch and a clean compile fails with:

[error] lmcoursier.internal.shaded.coursier.error.FetchError$DownloadingArtifacts: Error fetching artifacts:
[error] https://repo1.maven.org/maven2/com/google/protobuf/protoc/3.13.0/protoc-3.13.0-osx-aarch_64.exe: not found: https://repo1.maven.org/maven2/com/google/protobuf/protoc/3.13.0/protoc-3.13.0-osx-aarch_64.exe
[error] 
[error] 	at lmcoursier.internal.shaded.coursier.Artifacts$.$anonfun$fetchArtifacts$9(Artifacts.scala:365)
[error] 	at lmcoursier.internal.shaded.coursier.util.Task$.$anonfun$flatMap$extension$1(Task.scala:14)
[error] 	at lmcoursier.internal.shaded.coursier.util.Task$.$anonfun$flatMap$extension$1$adapted(Task.scala:14)
[error] 	at lmcoursier.internal.shaded.coursier.util.Task$.wrap(Task.scala:82)
[error] 	at lmcoursier.internal.shaded.coursier.util.Task$.$anonfun$flatMap$2(Task.scala:14)
[error] 	at scala.concurrent.Future.$anonfun$flatMap$1(Future.scala:307)
[error] 	at scala.concurrent.impl.Promise.$anonfun$transformWith$1(Promise.scala:41)
[error] 	at scala.concurrent.impl.CallbackRunnable.run(Promise.scala:64)
[error] 	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
[error] 	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
[error] 	at java.base/java.lang.Thread.run(Thread.java:829)
[error] Caused by: lmcoursier.internal.shaded.coursier.cache.ArtifactError$NotFound: not found: https://repo1.maven.org/maven2/com/google/protobuf/protoc/3.13.0/protoc-3.13.0-osx-aarch_64.exe
[error] 	at lmcoursier.internal.shaded.coursier.cache.internal.Downloader.checkErrFile$1(Downloader.scala:442)
[error] 	at lmcoursier.internal.shaded.coursier.cache.internal.Downloader.$anonfun$shouldDownload$19(Downloader.scala:499)
[error] 	at scala.concurrent.Future$.$anonfun$apply$1(Future.scala:659)
[error] 	at scala.util.Success.$anonfun$map$1(Try.scala:255)
[error] 	at scala.util.Success.map(Try.scala:213)
[error] 	at scala.concurrent.Future.$anonfun$map$1(Future.scala:292)
[error] 	at scala.concurrent.impl.Promise.liftedTree1$1(Promise.scala:33)
[error] 	at scala.concurrent.impl.Promise.$anonfun$transform$1(Promise.scala:33)
[error] 	at scala.concurrent.impl.CallbackRunnable.run(Promise.scala:64)
[error] 	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
[error] 	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
[error] 	at java.base/java.lang.Thread.run(Thread.java:829)
[error] (scio-core / protocExecutable) lmcoursier.internal.shaded.coursier.error.FetchError$DownloadingArtifacts: Error fetching artifacts:
[error] https://repo1.maven.org/maven2/com/google/protobuf/protoc/3.13.0/protoc-3.13.0-osx-aarch_64.exe: not found: https://repo1.maven.org/maven2/com/google/protobuf/protoc/3.13.0/protoc-3.13.0-osx-aarch_64.exe
...

It's odd, it seems to be defaulting to a lower version of Protobuf even when PB.protocVersion is explicitly set; I wonder if there's some issue with sbt-protoc's system-detection code? It looks like there was an issue filed for M1 support that got closed due to lack of activity.

I will do a little more investigation on the m1 thing on my end :)

@RustedBones
Copy link
Contributor Author

Right, the version needs to be set.
Can you share your error when using the newer version ?

@clairemcginty
Copy link
Contributor

Right, the version needs to be set. Can you share your error when using the newer version ?

yes that's what I meant - even with version set (the same way as in 6296beb), the plugin still tried to download protoc 3.13.0.

I realized the issue was that by putting PB.protocVersion in protobufSettings, it was only getting picked up by the few sbt subprojects that explicitly include protobufSettings:

scio % sbt protocVersion   
[info] welcome to sbt 1.7.1 (Homebrew Java 11.0.15)
...
[info] scio-cassandra3 / protocVersion
[info] 	3.13.0
[info] scio-jmh / protocVersion
[info] 	3.13.0
[info] scio-core / protocVersion
[info] 	3.13.0
[info] scio-repl / protocVersion
[info] 	3.13.0
[info] scio-elasticsearch6 / protocVersion
[info] 	3.13.0
[info] scio-test / protocVersion
[info] 	3.19.4
[info] scio-macros / protocVersion
[info] 	3.13.0
[info] scio-elasticsearch8 / protocVersion
[info] 	3.13.0
[info] scio-extra / protocVersion
[info] 	3.13.0
[info] scio-tensorflow / protocVersion
[info] 	3.19.4
[info] scio-elasticsearch7 / protocVersion
[info] 	3.13.0
[info] scio-jdbc / protocVersion
[info] 	3.13.0
[info] scio-parquet / protocVersion
[info] 	3.13.0
[info] scio-examples / protocVersion
[info] 	3.13.0
[info] scio-smb / protocVersion
[info] 	3.13.0
[info] scio-google-cloud-platform / protocVersion
[info] 	3.13.0
[info] scio-avro / protocVersion
[info] 	3.13.0
[info] scio-redis / protocVersion
[info] 	3.13.0
[info] protocVersion
[info] 	3.13.0

But sbt-protoc will try to eagerly download protocExecutable when compile is called, for every sbt subproject, and that's where the error comes from. So for scio-test/scio-tensorflow compilation succeeds but every other project fails. I was able to get it working by adding PB.protocVersion setting to commonSettings.

@RustedBones
Copy link
Contributor Author

Ha ok!
I think the trick here is to globally disable all auto-plugins and enable only for the desired projects. Will give this a shot

@RustedBones RustedBones marked this pull request as ready for review August 5, 2022 08:04
@RustedBones
Copy link
Contributor Author

@clairemcginty can you validate this again ?

@RustedBones RustedBones merged commit f26140a into main Aug 26, 2022
@RustedBones RustedBones deleted the sbt-protoc branch August 26, 2022 14:20
RustedBones added a commit that referenced this pull request Nov 22, 2022
* Migrate from sbt-protobuf to sbt-protoc.

Setup project so schemas can be generated in Test configuration

* Set proper protoc version

* Add managed sources for IDE

* Move protocVersion to global
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.

None yet

2 participants