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.

Sign up for GitHub

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

Jump to bottom

Make proto_library.proto_source_root work within multiple workspaces #4665

Closed
iirina opened this issue Feb 20, 2018 · 36 comments
Closed

Make proto_library.proto_source_root work within multiple workspaces #4665

iirina opened this issue Feb 20, 2018 · 36 comments
Assignees
@iirina @lberki
Labels
P1 I'll work on this now. (Assignee required) type: feature request

Comments

@iirina
Copy link
Contributor

iirina commented Feb 20, 2018

The current implementation of proto_library.proto_source_root is working only when there is one workspace. It should work with external repositories.
@iirina iirina added type: feature request P2 We'll consider working on this in future. (Assignee optional) category: rules > misc native labels Feb 20, 2018
@iirina iirina self-assigned this Feb 20, 2018
@iirina iirina mentioned this issue Feb 20, 2018
Closed
@or-shachar
Copy link
Contributor

or-shachar commented Oct 15, 2018 •
edited
Loading

@dslomov / @iirina - I believe I just had this issue with our private repos (that have cross-repo proto deps). How complicated is it to fix it?

@helenalt
Copy link
Contributor

This is currently a blocker for Wix migration - could we elevate the priority/get resolution please?

@ittaiz
Copy link
Member

ittaiz commented Oct 21, 2018

@iirina any update? we've changed plans a bit to work around this but we'll hit this wall soon again

@ittaiz
Copy link
Member

ittaiz commented Oct 26, 2018 via email

@iirina
Copy link
Contributor Author

iirina commented Oct 29, 2018

cc @lberki to prioritize this issue

@ittaiz
Copy link
Member

ittaiz commented Nov 3, 2018

@helenalt @dslomov on Monday 0.20 rc1 is supposed to be cut and it sounds like this won't make it. This means a very big setback for our plans since we can't use custom version on GCB due to perf hit.
Any suggestions?

@lberki lberki self-assigned this Nov 5, 2018
@lberki
Copy link
Contributor

lberki commented Nov 5, 2018

@iirina is OOO so I'm taking this over -- can you provide an example that does not work? I checked this with a trivial example:

t/WORKSPACE:

git_repository(name="com_google_protobuf", remote="https://github.com/google/protobuf", tag="v3.6.1")
local_repository(name="t2", path="<path to t2>")

t/p/BUILD:

proto_library(name="p", srcs=["e.proto"], deps=["@t2//p2"])

t/p/e.proto:

syntax = "proto3";

import "a/b/c.proto";

message E {
  C e = 1;
}

t2/WORKSPACE: <empty>
t2/p2/BUILD:

proto_library(
    name = "p2",
    srcs = [
	"a/b/c.proto",
	"a/b/d.proto",
    ],
    proto_source_root = "p2",
)

t2/a/b/c.proto:

import "a/b/d.proto";

message C {
  D c = 1;
}

t2/a/b/d.proto:

syntax = "proto3";

message D {
  string d = 1;
}

then in t, bazel build //p works as expected.

@lberki lberki added P1 I'll work on this now. (Assignee required) and removed P2 We'll consider working on this in future. (Assignee optional) labels Nov 5, 2018
@natansil
Copy link
Contributor

natansil commented Nov 5, 2018

@lberki Thanks for taking this.
I think the issue is with the providers of proto_library.
I will have a repro ready for you tomorrow.

@lberki
Copy link
Contributor

lberki commented Nov 6, 2018

Got it.

@philwo FYI -- if there is a straightforward fix, we'll need this in 0.20

@natansil
Copy link
Contributor

natansil commented Nov 6, 2018

@lberki I've managed to create a pretty simple repro here

TL;DR

scalapb_proto_library target build fails due to incorrect target.proto.transitive_proto_path which omits the external repo prefix.

This provider comes from proto_library dependency, I think with user input arg such as: proto_source_root = PACKAGE_NAME,

Explanation in detail:

it includes two repos/workspaces: repo_a and repo_b.
repo_a has a proto file that depends on a repo_b proto file.

building proto_library in repo_a succeeds:

$ bazel build //src/a_pkg:a_proto
INFO: Analysed target //src/a_pkg:a_proto (0 packages loaded).
INFO: Found 1 target...
INFO: From Generating Descriptor Set proto_library @b//src/b_pkg:b_proto:
src/b_pkg: warning: directory does not exist.
INFO: From Generating Descriptor Set proto_library //src/a_pkg:a_proto:
src/b_pkg: warning: directory does not exist.
Target //src/a_pkg:a_proto up-to-date:
  bazel-genfiles/src/a_pkg/a_proto-descriptor-set.proto.bin
INFO: Elapsed time: 0.225s, Critical Path: 0.10s
INFO: 2 processes: 2 darwin-sandbox.
INFO: Build completed successfully, 3 total actions

but building scalapb_proto_library fails:

$ bazel build //src/a_pkg:a_scala_proto
INFO: Analysed target //src/a_pkg:a_scala_proto (0 packages loaded).
INFO: Found 1 target...
INFO: From creating scalapb files //src/a_pkg:a_scala_proto_srcjar:
src/b_pkg: warning: directory does not exist.
some_package2/b.proto: File not found.
some_package/a.proto: Import "some_package2/b.proto" was not found or had errors.
some_package/a.proto:6:3: "Foo" seems to be defined in "src/b_pkg/some_package2/b.proto", which is not imported by "some_package/a.proto".  To use it here, please add the necessary import.
protoc-jar: protoc version: 3.6.0, detected platform: osx-x86_64 (mac os x/x86_64)
protoc-jar: embedded: bin/3.6.0/protoc-3.6.0-osx-x86_64.exe
protoc-jar: executing: [/var/folders/66/96g2fs910mb4kksv8znsmhghrs4sw9/T/protocjar8741178266753065176/bin/protoc.exe, 
--plugin=protoc-gen-scala=/var/folders/66/96g2fs910mb4kksv8znsmhghrs4sw9/T/protocbridge6948085968797357238, 
--scala_out=/var/folders/66/96g2fs910mb4kksv8znsmhghrs4sw9/T/bazelscalapb8865006782824716127, 
--proto_path=src/b_pkg, 
--proto_path=src/a_pkg, 
-Isrc/b_pkg/some_package2/b.proto=external/b/src/b_pkg/some_package2/b.proto, 
-Isrc/a_pkg/some_package/a.proto=src/a_pkg/some_package/a.proto, 
external/b/src/b_pkg/some_package2/b.proto, 
src/a_pkg/some_package/a.proto]
ERROR: /Users/natans/tmp/proto_source_root_many_repo/repo_a/src/a_pkg/BUILD.bazel:10:1: scala //src/a_pkg:a_scala_proto failed (Exit 1)
java.lang.RuntimeException: Must have input files from either source jars or local files.
	at io.bazel.rulesscala.scalac.ScalacProcessor.processRequest(ScalacProcessor.java:59)
	at io.bazel.rulesscala.worker.GenericWorker.run(GenericWorker.java:114)
	at io.bazel.rulesscala.scalac.ScalaCInvoker.main(ScalaCInvoker.java:41)
Target //src/a_pkg:a_scala_proto failed to build
Use --verbose_failures to see the command lines of failed build steps.
INFO: Elapsed time: 1.633s, Critical Path: 1.50s
INFO: 1 process: 1 darwin-sandbox.
FAILED: Build did NOT complete successfully

Notice that --proto_path=src/b_pkg is missing external/b/ prefix.
When I change target.proto.transitive_proto_path manualy in rules_scala to include external/b/src/b_pkg
then protoc gets --proto_path=external/b/src/b_pkg
and the build passes:

$ bazel build //src/a_pkg:a_scala_proto
INFO: Build options have changed, discarding analysis cache.
INFO: Analysed target //src/a_pkg:a_scala_proto (40 packages loaded).
INFO: Found 1 target...
INFO: From creating scalapb files //src/a_pkg:a_scala_proto_srcjar:
src/b_pkg: warning: directory does not exist.
protoc-jar: protoc version: 3.6.0, detected platform: osx-x86_64 (mac os x/x86_64)
protoc-jar: embedded: bin/3.6.0/protoc-3.6.0-osx-x86_64.exe
protoc-jar: executing: [/var/folders/66/96g2fs910mb4kksv8znsmhghrs4sw9/T/protocjar3260642254062231314/bin/protoc.exe, 
--plugin=protoc-gen-scala=/var/folders/66/96g2fs910mb4kksv8znsmhghrs4sw9/T/protocbridge6386601297548472659, 
--scala_out=/var/folders/66/96g2fs910mb4kksv8znsmhghrs4sw9/T/bazelscalapb7584903618422380669, 
--proto_path=src/a_pkg, 
--proto_path=external/b/src/b_pkg, 
-Ic.proto=external/b/c.proto, 
-Isrc/b_pkg/some_package2/b.proto=external/b/src/b_pkg/some_package2/b.proto, 
-Isrc/a_pkg/some_package/a.proto=src/a_pkg/some_package/a.proto, 
external/b/c.proto, 
external/b/src/b_pkg/some_package2/b.proto, 
src/a_pkg/some_package/a.proto]
Target //src/a_pkg:a_scala_proto up-to-date:
  bazel-bin/src/a_pkg/a_scala_proto.jar
  bazel-bin/src/a_pkg/a_scala_proto_java.jar
INFO: Elapsed time: 7.328s, Critical Path: 6.86s
INFO: 2 processes: 2 darwin-sandbox.
INFO: Build completed successfully, 4 total actions

Please let me know if you need more information
CC: @ittaiz @or-shachar

@ittaiz
Copy link
Member

ittaiz commented Nov 9, 2018 via email

@lberki
Copy link
Contributor

lberki commented Nov 9, 2018

/cc @dbabkin

Erm, sorry for dropping this on the floor. The first step is to admit that I am powerless over my inbox.

It appears that proto_library does the right thing as demonstrated by bazel build @b//src/b_pkg:b_proto working. It's "just" that scalapb_proto_library doesn't know about proto_source_root attributes and it doesn't protoc about them:

https://github.com/bazelbuild/rules_scala/blob/master/scala_proto/scala_proto.bzl#L367

Technically, the data is there:

https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoSourcesProvider.java#L111

so I think fixing this doesn't actually require changes to Bazel, only to rules_scala.

@lberki
Copy link
Contributor

lberki commented Nov 9, 2018

@ittaiz : turns out, I was wrong -- the data is not there in target.proto, only almost -- it has the set of transitive proto sources and the set of source roots but (crucially) without the external/ prefix. There are two possible options:

  1. Add the necessary information to target.proto in a hurry.
  2. Guess. That is, make _gen_proto_srcjar_impl look at target.proto.transitive_proto_path and target.proto.transitive_imports, estimate which proto is probably under which root and add the appropriate -I flags (or even just prefix the entries in --proto_path with external/<REPO> as appropriate.

TBH, given that (1) is a fire drill, I prefer (2) and then pick up the pieces later. That said, it is a decision we could regret later... would (2) even work in your use case? (it's obviously not perfect and if there are multiple repos with similar packages in them, its job would be hard)

@ittaiz
Copy link
Member

ittaiz commented Nov 9, 2018 via email

@lberki
Copy link
Contributor

lberki commented Nov 9, 2018

Well, it looks like you are writing the exact same logic as we have in Java :) :

https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilder.java#L691

And yes, we'll export this information soon -- this is only a band-aid.

@ittaiz
Copy link
Member

ittaiz commented Nov 9, 2018 via email

@natansil natansil mentioned this issue Nov 14, 2018
Closed
@lberki
Copy link
Contributor

lberki commented Nov 14, 2018 •
edited
Loading

Update: I have a change that almost fixes this, except that I'll have to figure out how not to break our internal version. Probably not an enormous task and is probably somewhere between "trivial" and "fiddly".

@natansil
Copy link
Contributor

Great news!
So this fix will make it to version 0.20 or 0.21?

@lberki
Copy link
Contributor

lberki commented Nov 15, 2018

To 0.20, hopefully!

I have a patch available: lberki@62e5daa

Can you take a look whether this is good enough for you? What it does is that it adds the appropriate entries dep.proto.transitive_proto_path so that .proto files are accessible without their external/REPONAME prefix. IOW, if you have a .proto file in repository foo, e.g. external/foo/bar/qux.proto, you'll get external/foo in dep.proto.transitive_proto_path.

At least @iirina 's test cases passes with this, so it can't be that bad.

/cc @dbabkin

@dslomov
Copy link
Contributor

dslomov commented Nov 15, 2018 via email

@natansil
Copy link
Contributor

Thank you @lberki I will check the patch on Sunday morning.
Do you recommend I patch above 0.19.1 or head?

@lberki
Copy link
Contributor

lberki commented Nov 16, 2018

I'd just clone lberki@62e5daa and build it. Patching on top of 0.19.1 works, too.

@natansil
Copy link
Contributor

natansil commented Nov 18, 2018 •
edited
Loading

@lberki I'm glad to report that the failing repro I've sent you earlier is now passing under your fix:

protoc-jar: executing: [/var/folders/66/96g2fs910mb4kksv8znsmhghrs4sw9/T/protocjar15176085434900894874/bin/protoc.exe, 
--plugin=protoc-gen-scala=/var/folders/66/96g2fs910mb4kksv8znsmhghrs4sw9/T/protocbridge9926809455046413317,
--scala_out=/var/folders/66/96g2fs910mb4kksv8znsmhghrs4sw9/T/bazelscalapb9171545577152976140, 
--proto_path=external/b/src/b_pkg, 
--proto_path=src/a_pkg, 
-Isrc/b_pkg/some_package2/b.proto=external/b/src/b_pkg/some_package2/b.proto, 
-Isrc/a_pkg/some_package/a.proto=src/a_pkg/some_package/a.proto, 
external/b/src/b_pkg/some_package2/b.proto, 
src/a_pkg/some_package/a.proto]

you can see that the path provided to the protoc-gen now includes the external/b prefix --proto_path=external/b/src/b_pkg

@lberki
Copy link
Contributor

lberki commented Nov 19, 2018

Yay! @dslomov , is this enough motivation for you to start reviewing the change? ;)

@lberki
Copy link
Contributor

lberki commented Nov 20, 2018 •
edited
Loading

@ittaiz : the change is still under review. Unfortunately, it's a bit of a breaking change. Would you be okay with running Bazel with a --incompatible_do_proto_library_less_awfully flag for a release?

@ittaiz
Copy link
Member

ittaiz commented Nov 20, 2018 via email

@lberki
Copy link
Contributor

lberki commented Nov 20, 2018

Got it -- let me just polish that change a bit then...

@lberki
Copy link
Contributor

lberki commented Nov 23, 2018

This should work at HEAD. It's still possible that the change will get rolled back if my efforts to make it backwards compatible did not succeed, so I'm keeping this bug open for the time being.

Could you verify that HEAD works for you in case the changes during the code review broke it?

@lberki lberki reopened this Nov 23, 2018
@natansil
Copy link
Contributor

Sure, I'll test this hopefully by Sunday night

@natansil
Copy link
Contributor

@lberki it indeed works with bazel HEAD.

@lberki
Copy link
Contributor

lberki commented Nov 26, 2018

Update: this change breaks a lot of things internally at Google, and rules_go and buildtools. Unfortunately, a rollback is not feasible due to the other changes I made in the area recently, so I'll improvise -- the internal issues are easy enough. to fix and I hope that rules_go and buildtools have the same root cause.

bazel-io pushed a commit that referenced this issue Nov 26, 2018
@lberki
Remove --proto_path=. from protoc command lines.
af36058 
This broke a lot of things.  For lack of proto knowledge and the unfeasibility of a rollback, I'll stick to symptomatic treatment and remove that argument from the command line. It should have been done that way anyway, except that I was too optimistic :(

Progress of #4665.

RELNOTES: None.
PiperOrigin-RevId: 222784471
@natansil
Copy link
Contributor

Thank you for the update. Please keep us updated and good luck!

@lberki
Copy link
Contributor

lberki commented Nov 26, 2018

Update: this is fixed at HEAD.

@natansil
Copy link
Contributor

👍

@RNabel RNabel mentioned this issue Apr 14, 2019
Closed
@RNabel
Copy link
Contributor

RNabel commented Apr 14, 2019

@lberki If I'm not mistaken, this issue can be closed.

@lberki
Copy link
Contributor

lberki commented Jun 4, 2019

Oh. Indeed.

@lberki lberki closed this as completed Jun 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@iirina iirina

@lberki lberki

Labels
P1 I'll work on this now. (Assignee required) type: feature request
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
8 participants
@ittaiz @iirina @or-shachar @lberki @dslomov @helenalt @RNabel @natansil