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

fix: java generation #673

Closed
wants to merge 1 commit into from
Closed

Conversation

vam-google
Copy link
Contributor

We had to update packaging rules in bazel build recently. The change was supposed to be backward-compatible, but the way synth was unpacking the tar made it backward-incompatible.

Specifically, previously the final .tar.gz file produced by bazel packaged files in the following format (i.e. this is how the output of tar -tvf google-cloud-oslogin-v1-java.tar.gz would look like, using oslogin as an example):

./google-cloud-oslogin-v1-java/gapic-google-cloud-oslogin-v1-java/some/files/here1
./google-cloud-oslogin-v1-java/proto-google-cloud-oslogin-v1-java/some/files/here2
./google-cloud-oslogin-v1-java/grpc-google-cloud-oslogin-v1-java/some/files/here2
./google-cloud-oslogin-v1-java/build/scripts

Witht the recent change the output of the same build target will look like:

google-cloud-oslogin-v1-java/gapic-google-cloud-oslogin-v1-java/some/files/here1
google-cloud-oslogin-v1-java/proto-google-cloud-oslogin-v1-java/some/files/here2
google-cloud-oslogin-v1-java/grpc-google-cloud-oslogin-v1-java/some/files/here2
google-cloud-oslogin-v1-java/build/scripts

I.e. they should be identical essentially, because the ./ prefix in the first example is a no-op component in the path when you unpack tar normally. But it still counts as a "component" if "--strip-component=1" command line arg is used.

Due to technical reasons adding back the ./ prefix in bazel is difficult and would make the rules inconsistent, adding it to synthtool instead (since the bug is essentially reproducible only if --strip-component arg is used).

We had to update packaging rules in bazel build recently. The change was supposed to be backward-compatible, but the way synth was unpacking the tar made it backward-incompatible.

Specifically, previously the final `.tar.gz` file produced by bazel packaged files in the following format (i.e. this is how the output of `tar -tvf google-cloud-oslogin-v1-java.tar.gz` would look like, using oslogin as an example):
```
./google-cloud-oslogin-v1-java/gapic-google-cloud-oslogin-v1-java/...
```

Witht the recent change the output of the same build target will look like:
```
google-cloud-oslogin-v1-java/gapic-google-cloud-oslogin-v1-java/...
```

I.e. they should be identical essentially, because the `./` prefix in the first example is a no-op for unpacking. But it still counts as a "component" if `"--strip-component=1"` command line arg is used.

Due to technical reasons adding back the `./` prefix in bazel is difficult and would make the rules inconsistent, adding it to synthtool instead.
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jul 17, 2020
@vam-google
Copy link
Contributor Author

@chingor13 PTAL.
I also have no idea what is wrong with the message format (to me it looks like it follows conventional commits).

@vam-google
Copy link
Contributor Author

There are also related changes to fix stuff on mac googleapis/gapic-generator#3255 (not required for linux builds)

Copy link
Contributor

@chingor13 chingor13 left a comment

Choose a reason for hiding this comment

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

Do we know why this only affects Java? It seems like each language would have a similar assembly target and would be equally affected.

@@ -190,7 +190,7 @@ def _generate_code(
"tar",
"-C",
str(output_dir),
"--strip-components=1",
"--strip-components={}".format("0" if language == "java" else "1"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the terrific PR description.

I see this will fix it, but I'm concerned that synthtool is so fragile that it can't accept the small change in paths. I'd like to investigate why synthtool is choking in the new paths for an hour or two.

@SurferJeffAtGoogle
Copy link
Contributor

I tried generating java-asset, and I see the path being passed to move() is:
PosixPath('/tmp/tmpiry_e6j0/google-cloud-asset-v1-java/gapic-google-cloud-asset-v1-java/src')

However, the directories that were created were:

gapic-google-cloud-asset-v1-java
grpc-google-cloud-asset-v1-java
proto-google-cloud-asset-v1-java

@SurferJeffAtGoogle
Copy link
Contributor

So, specifically, the directory /tmp/tmpiry_e6j0/google-cloud-asset-v1-java does not exist.

@SurferJeffAtGoogle
Copy link
Contributor

fixed with alternative code #675

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants