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

ci: avoid fedora's pkgconf which is slow #7061

Merged
merged 1 commit into from
Jul 26, 2021
Merged

Conversation

devjgm
Copy link
Contributor

@devjgm devjgm commented Jul 26, 2021

Fixes: #7052

This avoids Fedora's pkgconf (a drop-in replacement for pkg-config)
because it's too slow when dealing with .pc files with lots of
Requires: deps, which we have with Abseil. Instead, we use the normal
pkg-config binary, which seems to work better.

After this PR, we should be able to upgrade to gRPC 1.39.0 (git revert ba41d5a99818922b09eee5da148786c1d68c17f5)


This change is Reviewable

Fixes: googleapis#7052

This avoids Fedora's `pkgconf` (a drop-in replacement for `pkg-config`)
because it's too slow when dealing with `.pc` files with lots of
`Requires:` deps, which we have with Abseil. Instead, we use the normal
`pkg-config` binary, which seems to work better.

After this PR, we should be able to upgrade to gRPC 1.39.0 (`git revert
ba41d5a`)
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jul 26, 2021
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 26, 2021
@google-cloud-cpp-bot
Copy link
Collaborator

Google Cloud Build Logs
For commit: e525827882edf1b22fb9bfc36fe5a7b72e1a1143

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

@@ -149,6 +149,7 @@ done < <(find "${INSTALL_PREFIX}" -type f -print0)

for repo_root in "ci/verify_current_targets" "ci/verify_deprecated_targets"; do
out_dir="cmake-out/$(basename "${repo_root}")-out"
rm -f "${out_dir}/CMakeCache.txt"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is so that we can run the cmake-install build multiple times on a machine where this file is persistent (e.g., our --docker builds). The problem is that this file caches the temporary install location from the previous invocation.

Copy link
Contributor

Choose a reason for hiding this comment

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

For a separate PR: maybe the install location should not be temporary? /h/test-install or cmake-out/test-install?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we want a fully clean install each time. Otherwise, if we removed an artifact in one build, the artifact may still exist on disk and we may not notice that we're still depending on the removed artifact.... well, our CI builds in GCB would notice, but we wouldn't notice on our local machines until we blew away the install directory and rebuilt. Anyway, that's why I originally installed in a new dir every time.

@devjgm devjgm marked this pull request as ready for review July 26, 2021 21:34
@devjgm devjgm requested a review from a team as a code owner July 26, 2021 21:34
@codecov
Copy link

codecov bot commented Jul 26, 2021

Codecov Report

Merging #7061 (e525827) into main (b10aadd) will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #7061      +/-   ##
==========================================
- Coverage   94.49%   94.48%   -0.01%     
==========================================
  Files        1304     1304              
  Lines      112327   112327              
==========================================
- Hits       106140   106134       -6     
- Misses       6187     6193       +6     
Impacted Files Coverage Δ
...bigtable/examples/bigtable_hello_instance_admin.cc 81.31% <0.00%> (-2.20%) ⬇️
...le/cloud/internal/default_completion_queue_impl.cc 97.00% <0.00%> (-0.60%) ⬇️
google/cloud/pubsub/samples/samples.cc 91.75% <0.00%> (-0.24%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b10aadd...e525827. Read the comment docs.

@@ -149,6 +149,7 @@ done < <(find "${INSTALL_PREFIX}" -type f -print0)

for repo_root in "ci/verify_current_targets" "ci/verify_deprecated_targets"; do
out_dir="cmake-out/$(basename "${repo_root}")-out"
rm -f "${out_dir}/CMakeCache.txt"
Copy link
Contributor

Choose a reason for hiding this comment

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

For a separate PR: maybe the install location should not be temporary? /h/test-install or cmake-out/test-install?

@devjgm devjgm merged commit 5ecd239 into googleapis:main Jul 26, 2021
@devjgm devjgm deleted the upgrade-grpc branch July 26, 2021 21:59
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.

Upgrading to grpc 1.39.0 makes our cmake-install build too slow
4 participants