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

Windows-specific filegroup to capture envoy-static.pdb output file from opt build #16043

Merged
merged 5 commits into from
Oct 12, 2021

Conversation

wrowe
Copy link
Contributor

@wrowe wrowe commented Apr 16, 2021

Commit Message: Windows-specific filegroup to capture envoy-static.pdb output file from opt build
Additional Description:

  • Adds a windows-specific envoy-static-exe target that adds the pdb_file group to the default group
  • Move the pdb output into the debug image

See bazelbuild/rules_cc#94 for details

Signed-off-by: William A Rowe Jr [email protected]

Risk Level: low
Testing: local msvc + clang-cl in process across opt/dbg/fastbuild
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: Windows target only

@wrowe wrowe marked this pull request as draft April 16, 2021 21:48
@wrowe
Copy link
Contributor Author

wrowe commented Apr 16, 2021

Lots of tests yet to complete - offering this up to @envoyproxy/windows-dev for early access

@htuch
Copy link
Member

htuch commented Apr 18, 2021

/assign-from @envoyproxy/windows-dev

@repokitteh-read-only
Copy link

@envoyproxy/windows-dev assignee is @sunjayBhatia

🐱

Caused by: a #16043 (comment) was created by @htuch.

see: more, trace.

@wrowe wrowe marked this pull request as ready for review April 19, 2021 06:26
@wrowe
Copy link
Contributor Author

wrowe commented Apr 19, 2021

Pleased to report this succeeds agnostically in dbg and fastbuild builds (where pdb is captured) as well as opt (where pdb is discarded by default.) @davinci26 I see you are also working on images and mentioned azure, so please have a look.

In terms of distinct packages from Windows for only the .exe, or combo .exe+.pdb, I am wondering whether .zip files make more sense for distribution (as Extract-Archive is baked in to PowerShell), but that can even be a separate discussion, this workaround mentioned in bazelbuild/rules_cc#94 solves (and could be documented, at the bazel project.)

@wrowe wrowe changed the title Experimental filegroup to capture opt build pdb output Windows-specific filegroup to capture envoy-static.pdb output file from opt build Apr 19, 2021
@sunjayBhatia
Copy link
Member

sunjayBhatia commented Apr 19, 2021

Looks like theres still an issue with getting the .exe output with this change: https://dev.azure.com/cncf/envoy/_build/results?buildId=72410&view=logs&j=b840a642-5ff3-5357-2e4b-e06e40b0cffd&t=67965174-5100-5631-9dc0-68b9f0aacb53&l=903

@wrowe
Copy link
Contributor Author

wrowe commented Apr 19, 2021

Looks like theres still an issue with getting the .exe output

Let's try stacking cc_binary+filegroup, since there appears to be no way to solve this in a single filegroup, especially without confusing build source refs.

@wrowe
Copy link
Contributor Author

wrowe commented Apr 20, 2021

No behaviors working as-expected, I've re-escalated to bazel team.

@wrowe wrowe marked this pull request as draft April 20, 2021 00:59
@wrowe
Copy link
Contributor Author

wrowe commented Apr 20, 2021

Pleased to report this succeeds agnostically in dbg and fastbuild builds

Strike my previous report. We are not going to be able to trust local builds, as we work out the right fix, we will have to bump through 6 CI passes (fastbuild dbg opt against msvc, clang-cl) before declaring it resolved.

@wrowe
Copy link
Contributor Author

wrowe commented Apr 27, 2021

The dbg build fails in a predictable ways (Release - msvc / clang-cl);

 //test/extensions/filters/http/lua:lua_filter_test                       FAILED in 2 out of 2 in 24.3s
 //test/server:filter_chain_benchmark_test_benchmark_test                 FAILED in 2 out of 2 in 9.5s
 //test/integration:integration_test                                       FLAKY, failed in 1 out of 3 in 303.2s
 //test/extensions/filters/http/lua:lua_filter_test                       FAILED in 2 out of 2 in 32.8s
 //test/server:filter_chain_benchmark_test_benchmark_test                 FAILED in 2 out of 2 in 11.1s
 //test/extensions/filters/http/wasm:wasm_filter_test                      FLAKY, failed in 1 out of 51 in 11.6s

The fastbuild build fails similarly (Release - msvc / clang-cl);

 //test/server:server_test                                               TIMEOUT in 2 out of 2 in 304.0s
 //test/extensions/filters/http/lua:lua_filter_test                       FAILED in 2 out of 2 in 17.9s
 //test/extensions/http/header_formatters/preserve_case:preserve_case_formatter_integration_test FAILED in 2 out of 2 in 5.2s
 //test/extensions/transport_sockets/tls/cert_validator/spiffe:spiffe_validator_integration_test FAILED in 2 out of 2 in 5.7s
 //test/integration:tcp_proxy_integration_test                             FLAKY, failed in 1 out of 3 in 303.5s
 //test/integration:multiplexed_integration_test                          FAILED in 2 out of 5 in 32.1s
 //test/server:server_test                                               TIMEOUT in 2 out of 2 in 303.9s
 //test/extensions/filters/http/lua:lua_filter_test                       FAILED in 2 out of 2 in 14.7s
 //test/extensions/http/header_formatters/preserve_case:preserve_case_formatter_integration_test FAILED in 2 out of 2 in 5.9s
 //test/extensions/transport_sockets/tls/cert_validator/spiffe:spiffe_validator_integration_test FAILED in 2 out of 2 in 5.1s
 //test/integration:tcp_proxy_integration_test                             FLAKY, failed in 1 out of 3 in 303.1s
 //test/integration:multiplexed_integration_test                          FAILED in 3 out of 6 in 29.3s

The code is expected to behave differently when not optimized, but the .exe and .pdb files were both emitted in all cases, so this workaround is confirmed to not be disruptive. Reverting to opt for review.

@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label May 10, 2021
@repokitteh-read-only
Copy link

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).

🐱

Caused by: #16043 was synchronize by wrowe.

see: more, trace.

@wrowe
Copy link
Contributor Author

wrowe commented May 24, 2021

Can't take this any further, we will have to rebuild the toolchains, and must have a tag on bazel inclusive of this change... so it's on pause.

@wrowe
Copy link
Contributor Author

wrowe commented Jun 4, 2021

The bazel fix has made it into 5.0.0-pre.20210510.2 (and .2.2) but did not make it into 4.1.0 and is not yet present in a 4.1.x candidate, if one is eventually created.

@wrowe
Copy link
Contributor Author

wrowe commented Jun 9, 2021

This continues to be blocked on bazel release 4.2 for commit bazelbuild/bazel@6dc941e and needs a corresponding rules_cc bump.

@github-actions
Copy link

github-actions bot commented Jul 9, 2021

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Jul 9, 2021
@wrowe wrowe added no stalebot Disables stalebot from closing an issue and removed stale stalebot believes this issue/PR has not been touched recently labels Jul 13, 2021
yanavlasov and others added 4 commits October 4, 2021 11:30
Signed-off-by: Yan Avlasov <[email protected]>
Signed-off-by: Yan Avlasov <[email protected]>
…tput

See bazelbuild/rules_cc#94 for details

Use generate_pdb_file feature to capture pdb from envoy-static.exe

- Introduces features list arg to envoy_cc_binary
- Toggles the feature only for the windows opt case in the envoy-static target
- Capture the resulting envoy-static.pdb output

Signed-off-by: William A Rowe Jr <[email protected]>
Drops clang-cl which is presently broken

Signed-off-by: William A Rowe Jr <[email protected]>
@wrowe wrowe marked this pull request as ready for review October 6, 2021 18:41
@wrowe wrowe requested review from lizan and davinci26 October 6, 2021 18:41
@wrowe
Copy link
Contributor Author

wrowe commented Oct 6, 2021

With the updated bazel-toolchain images provisioned and bazel 4.2.1 out, this is now ready for review.

Copy link
Member

@davinci26 davinci26 left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for driving this across so many layers. One small question

ci/windows_ci_steps.sh Show resolved Hide resolved
@davinci26
Copy link
Member

I have approved so I will defer to @lizan for the final review/merge

@davinci26
Copy link
Member

davinci26 commented Oct 12, 2021

This could be merged but since its windows PR, I would also like one reviewer outside of the windows devs.

Lizan is OOO for KubeCon this week.

/assign-from @envoyproxy/senior-maintainers (edit: after my trick failed, I assigned it to Matt)

@repokitteh-read-only
Copy link

@envoyproxy/senior-maintainers assignee is @lizan

🐱

Caused by: a #16043 (comment) was created by @davinci26.

see: more, trace.

@wrowe wrowe requested a review from mattklein123 October 12, 2021 20:11
@mattklein123 mattklein123 merged commit 6252a1e into envoyproxy:main Oct 12, 2021
@wrowe wrowe deleted the pdb-hack branch October 12, 2021 23:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build area/windows deps Approval required for changes to Envoy's external dependencies no stalebot Disables stalebot from closing an issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants