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

Use param files with go-protoc #3190

Merged
merged 1 commit into from
Jun 8, 2022
Merged

Use param files with go-protoc #3190

merged 1 commit into from
Jun 8, 2022

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Jun 8, 2022 •
edited
Loading

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?

Makes the GoProtocGen action use parameter files if the argument list gets too long.

Which issues(s) does this PR fix?

Fixes #3188

Other notes for review

Tested manually by temporarily setting use_always = True and running the tests.

@fmeum fmeum mentioned this pull request Jun 8, 2022
Closed
@fmeum
Copy link
Collaborator Author

fmeum commented Jun 8, 2022

@linzhp Could you try this at Uber?

@sateeshpnv
Copy link

Always using the param file can cause the build to slow down because of the file IO overhead. May not be visible in small projects. However, with 100's of dependencies it can get amplified.

In any case, I verified with this fix and it still fails with "argument list too long"

@fmeum
Copy link
Collaborator Author

fmeum commented Jun 8, 2022

@sateeshpnv use_param_files without further keyword arguments only tells Bazel how to use a param file, it doesn't force it to use param files in all cases. That is what the (unset by default) parameter use_always is for.

Could you share logs of the failed command line, ideally with -s?

@sateeshpnv
Copy link

sateeshpnv commented Jun 8, 2022 •
edited
Loading

@fmeum,

Firstly, thanks a lot for looking into it so quickly.

It fails with

  bazel-out/k8-opt-exec-2B5CBBC6/bin/external/io_bazel_rules_go/go/tools/builders/go-protoc/go-protoc-bin '-param=bazel-out/k8-fastbuild/bin/idl/....pb.go-0.params')

...

2022/06/08 10:32:09 error running protoc: fork/exec bazel-out/k8-opt-exec-2B5CBBC6/bin/rules/proto/protoc: argument list too long

Content of the params file:

-protoc
bazel-out/k8-opt-exec-2B5CBBC6/bin/rules/proto/protoc
-importpath
...

Though param file is used by go-protoc-bin, I guess the problem is - the native protoc binary is still executed with a very long argument list.

Please see the #3191, which is working; not sure if that is the most elegant way, though.

@fmeum
Copy link
Collaborator Author

fmeum commented Jun 8, 2022 •
edited
Loading

@sateeshpnv Could you give this another try? I updated it based on your PR. Instead of a hardcoded limit, it uses a param file for protoc if Bazel uses one for go-protoc.

@sateeshpnv
Copy link

@fmeum ,

It works now. Thank you.

@fmeum
Copy link
Collaborator Author

fmeum commented Jun 8, 2022

@sateeshpnv Thanks for confirming. Could you close your other PR? Thanks again for the report and the fix!

linzhp
linzhp approved these changes Jun 8, 2022
View reviewed changes
Copy link
Contributor

@linzhp linzhp left a comment

Choose a reason for hiding this comment

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

Because @sateeshpnv is from Uber, I believe he has tested this PR in Go monorepo :)

go/tools/builders/env.go Show resolved Hide resolved
@fmeum
Use param files with go-protoc
42cc0fe 
If Bazel uses a param file for the args to go-protoc, go-protoc will
in turn use a param file for protoc.
@sateeshpnv sateeshpnv mentioned this pull request Jun 8, 2022
Closed
@linzhp linzhp merged commit 84ce4e5 into bazelbuild:master Jun 8, 2022
gcf-merge-on-green bot pushed a commit to googleapis/gapic-generator-go that referenced this pull request Jul 19, 2022
@renovate-bot
chore(deps): update dependency io_bazel_rules_go to v0.34.0 (#1076)
22aecf2 
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [io_bazel_rules_go](https://togithub.com/bazelbuild/rules_go) | http_archive | minor | `v0.33.0` -> `v0.34.0` |

---

### Release Notes

<details>
<summary>bazelbuild/rules_go</summary>

### [`v0.34.0`](https://togithub.com/bazelbuild/rules_go/releases/tag/v0.34.0)

[Compare Source](https://togithub.com/bazelbuild/rules_go/compare/v0.33.0...v0.34.0)

#### What's Changed

-   releaser: fix scrubbing timestamp from patch files by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3180
-   Replace Starlark JSON parser with json.decode by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3184
-   gopackagesdriver: separates "s" files in pkg info by [@&#8203;iamricard](https://togithub.com/iamricard) in [bazelbuild/rules_go#3165
-   Refactor away references to @&#8203;io_bazel_rules_go by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3185
-   Do not print to stderr if cgo linking succeeds after retry by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3187
-   Use param files with go-protoc by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3190
-   Don't include non-executable go_binary in dependent's runfiles by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3151
-   Link in native libraries of transitive dependencies in archive mode by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3186
-   runfiles: remove deprecated api by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3198
-   Fix failing open hermeticity test by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3206
-   Fix go_googleapis Gazelle patch by [@&#8203;nickgooding](https://togithub.com/nickgooding) in [bazelbuild/rules_go#3193
-   Exclude unsupported C/C++ features by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3189
-   Allow gomock to take Bazel common attributes by [@&#8203;linzhp](https://togithub.com/linzhp) in [bazelbuild/rules_go#3207
-   Transition on edges not self by [@&#8203;illicitonion](https://togithub.com/illicitonion) in [bazelbuild/rules_go#3116
-   Include go_transition_test in bazel aspect by [@&#8203;ian-h-chamberlain](https://togithub.com/ian-h-chamberlain) in [bazelbuild/rules_go#3160
-   Add an example for go_download_sdk.sdks by [@&#8203;fishy](https://togithub.com/fishy) in [bazelbuild/rules_go#3139
-   tests: nogo over generated code by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3214
-   test nogo/coverage: test generated code by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3213
-   Remove references to go_transition_test by [@&#8203;linzhp](https://togithub.com/linzhp) in [bazelbuild/rules_go#3215
-   Basic bzlmod setup by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3047
-   Run BCR tests against Bazel 6.0.0-pre.20220608.2 by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3223
-   Use repo-relative labels in MODULE.bazel by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3226
-   upkeep: upgrade to go 1.18.3 and gazelle v0.26.0 by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3220
-   nogo: ignore generated source files by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3216
-   asm: Pass package path with -p by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3231
-   bzlmod: Add support for gomock by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3232
-   test cgo: ensure helper script works by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3236
-   Fix //tests/legacy/examples/cgo:cgo_lib_test on M1 Macs by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3237
-   gopackagesdriver: Descend into go_proto_compiler's deps by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3240
-   new_library: remove unused resolver by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3219
-   nogo: instantiate type info for generic types when running under Go >=1.18 by [@&#8203;farhaven](https://togithub.com/farhaven) in [bazelbuild/rules_go#3212

#### New Contributors

-   [@&#8203;iamricard](https://togithub.com/iamricard) made their first contribution in [bazelbuild/rules_go#3165
-   [@&#8203;ian-h-chamberlain](https://togithub.com/ian-h-chamberlain) made their first contribution in [bazelbuild/rules_go#3160
-   [@&#8203;fishy](https://togithub.com/fishy) made their first contribution in [bazelbuild/rules_go#3139
-   [@&#8203;farhaven](https://togithub.com/farhaven) made their first contribution in [bazelbuild/rules_go#3212

**Full Changelog**: bazelbuild/rules_go@v0.33.0...v0.34.0

#### `WORKSPACE` code

    load("@&#8203;bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")

    http_archive(
        name = "io_bazel_rules_go",
        sha256 = "16e9fca53ed6bd4ff4ad76facc9b7b651a89db1689a2877d6fd7b82aa824e366",
        urls = [
            "https://mirror.bazel.build/github.com/bazelbuild/rules_go/releases/download/v0.34.0/rules_go-v0.34.0.zip",
            "https://github.com/bazelbuild/rules_go/releases/download/v0.34.0/rules_go-v0.34.0.zip",
        ],
    )

    load("@&#8203;io_bazel_rules_go//go:deps.bzl", "go_register_toolchains", "go_rules_dependencies")

    go_rules_dependencies()

    go_register_toolchains(version = "1.18.4")

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/gapic-generator-go).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4xMTcuNCIsInVwZGF0ZWRJblZlciI6IjMyLjExNy40In0=-->
gcf-merge-on-green bot pushed a commit to googleapis/gapic-config-validator that referenced this pull request Jul 19, 2022
@renovate-bot
chore(deps): update dependency io_bazel_rules_go to v0.34.0 (#255)
b92b5b7 
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [io_bazel_rules_go](https://togithub.com/bazelbuild/rules_go) | http_archive | minor | `v0.33.0` -> `v0.34.0` |

---

### Release Notes

<details>
<summary>bazelbuild/rules_go</summary>

### [`v0.34.0`](https://togithub.com/bazelbuild/rules_go/releases/tag/v0.34.0)

[Compare Source](https://togithub.com/bazelbuild/rules_go/compare/v0.33.0...v0.34.0)

#### What's Changed

-   releaser: fix scrubbing timestamp from patch files by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3180
-   Replace Starlark JSON parser with json.decode by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3184
-   gopackagesdriver: separates "s" files in pkg info by [@&#8203;iamricard](https://togithub.com/iamricard) in [bazelbuild/rules_go#3165
-   Refactor away references to @&#8203;io_bazel_rules_go by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3185
-   Do not print to stderr if cgo linking succeeds after retry by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3187
-   Use param files with go-protoc by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3190
-   Don't include non-executable go_binary in dependent's runfiles by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3151
-   Link in native libraries of transitive dependencies in archive mode by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3186
-   runfiles: remove deprecated api by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3198
-   Fix failing open hermeticity test by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3206
-   Fix go_googleapis Gazelle patch by [@&#8203;nickgooding](https://togithub.com/nickgooding) in [bazelbuild/rules_go#3193
-   Exclude unsupported C/C++ features by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3189
-   Allow gomock to take Bazel common attributes by [@&#8203;linzhp](https://togithub.com/linzhp) in [bazelbuild/rules_go#3207
-   Transition on edges not self by [@&#8203;illicitonion](https://togithub.com/illicitonion) in [bazelbuild/rules_go#3116
-   Include go_transition_test in bazel aspect by [@&#8203;ian-h-chamberlain](https://togithub.com/ian-h-chamberlain) in [bazelbuild/rules_go#3160
-   Add an example for go_download_sdk.sdks by [@&#8203;fishy](https://togithub.com/fishy) in [bazelbuild/rules_go#3139
-   tests: nogo over generated code by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3214
-   test nogo/coverage: test generated code by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3213
-   Remove references to go_transition_test by [@&#8203;linzhp](https://togithub.com/linzhp) in [bazelbuild/rules_go#3215
-   Basic bzlmod setup by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3047
-   Run BCR tests against Bazel 6.0.0-pre.20220608.2 by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3223
-   Use repo-relative labels in MODULE.bazel by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3226
-   upkeep: upgrade to go 1.18.3 and gazelle v0.26.0 by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3220
-   nogo: ignore generated source files by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3216
-   asm: Pass package path with -p by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3231
-   bzlmod: Add support for gomock by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3232
-   test cgo: ensure helper script works by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3236
-   Fix //tests/legacy/examples/cgo:cgo_lib_test on M1 Macs by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3237
-   gopackagesdriver: Descend into go_proto_compiler's deps by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3240
-   new_library: remove unused resolver by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3219
-   nogo: instantiate type info for generic types when running under Go >=1.18 by [@&#8203;farhaven](https://togithub.com/farhaven) in [bazelbuild/rules_go#3212

#### New Contributors

-   [@&#8203;iamricard](https://togithub.com/iamricard) made their first contribution in [bazelbuild/rules_go#3165
-   [@&#8203;ian-h-chamberlain](https://togithub.com/ian-h-chamberlain) made their first contribution in [bazelbuild/rules_go#3160
-   [@&#8203;fishy](https://togithub.com/fishy) made their first contribution in [bazelbuild/rules_go#3139
-   [@&#8203;farhaven](https://togithub.com/farhaven) made their first contribution in [bazelbuild/rules_go#3212

**Full Changelog**: bazelbuild/rules_go@v0.33.0...v0.34.0

#### `WORKSPACE` code

    load("@&#8203;bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")

    http_archive(
        name = "io_bazel_rules_go",
        sha256 = "16e9fca53ed6bd4ff4ad76facc9b7b651a89db1689a2877d6fd7b82aa824e366",
        urls = [
            "https://mirror.bazel.build/github.com/bazelbuild/rules_go/releases/download/v0.34.0/rules_go-v0.34.0.zip",
            "https://github.com/bazelbuild/rules_go/releases/download/v0.34.0/rules_go-v0.34.0.zip",
        ],
    )

    load("@&#8203;io_bazel_rules_go//go:deps.bzl", "go_register_toolchains", "go_rules_dependencies")

    go_rules_dependencies()

    go_register_toolchains(version = "1.18.4")

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/gapic-config-validator).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4xMTcuNCIsInVwZGF0ZWRJblZlciI6IjMyLjExNy40In0=-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@linzhp linzhp linzhp approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

protoc argument list too long
3 participants
@fmeum @sateeshpnv @linzhp