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

update to 1.62.2 #31

Merged
merged 439 commits into from
May 27, 2024
Merged

update to 1.62.2 #31

merged 439 commits into from
May 27, 2024

Conversation

cbouss
Copy link

@cbouss cbouss commented May 14, 2024

h-vetinari and others added 30 commits August 13, 2022 11:15
automerged PR by conda-forge/automerge-action
…-c-ares1-0-1_h685e74

Rebuild for c-ares1
automerged PR by conda-forge/automerge-action
automerged PR by conda-forge/automerge-action
automerged PR by conda-forge/automerge-action
Copy link

@skupr-anaconda skupr-anaconda left a comment

Choose a reason for hiding this comment

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

LGTM but a clarifying question about protobuf for grpcio

- wheel >=0.29
- abseil-cpp 20230802.0
- c-ares 1.19.1
- libprotobuf {{ libprotobuf }}

Choose a reason for hiding this comment

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

Is protobuf required for building grpcio? Or it's only for examples and/or testing?
See https://github.com/grpc/grpc/blob/v1.62.2/requirements.txt#L4

Choose a reason for hiding this comment

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

Not from what I see.

- pkg-config
files:
- tests/include/*
- cmake_test/

Choose a reason for hiding this comment

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

I'm failing to see the difference between files and source_files? Shouldn't hello.proto be part of source_files?

Copy link
Author

Choose a reason for hiding this comment

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

- ninja
- cmake
- ninja-base
# During cross-compilation, we need to build the grpc_cpp_plugin for the

Choose a reason for hiding this comment

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

maybe I'm missing something, but when do we do cross compilation?

Copy link
Author

Choose a reason for hiding this comment

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

We don't, but most changes here are synced from conda-forge.

- ninja
- cmake
- ninja-base
# During cross-compilation, we need to build the grpc_cpp_plugin for the

Choose a reason for hiding this comment

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

maybe I'm missing something, but when do we do cross compilation?

Copy link
Author

Choose a reason for hiding this comment

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

We don't, but most changes here are synced from conda-forge.

Copy link

@JeanChristopheMorinPerso JeanChristopheMorinPerso left a comment

Choose a reason for hiding this comment

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

Looks good to me. I went though the build logs for linux-64, osx-arm64 and win-64, looked at the DSO analysis, looked at the cython compiler flags and I also took a quick look at the installed files. I also inspected the builds scripts and nothing in particular catched my attention.

I did notice https://github.com/grpc/grpc/blob/v1.62.2/setup.py#L299-L322 on the setup.py though. But I think it should be mostly fine. Same for https://github.com/grpc/grpc/blob/v1.62.2/setup.py#L53-L81.

I don't think we necessarily need to fix this right now. It could well be done in another PR (since this PR is already big enough). I think it's something we could also contribute back in the conda-forge feedstock.

@cbouss
Copy link
Author

cbouss commented May 24, 2024

Looks good to me. I went though the build logs for linux-64, osx-arm64 and win-64, looked at the DSO analysis, looked at the cython compiler flags and I also took a quick look at the installed files. I also inspected the builds scripts and nothing in particular catched my attention.

I did notice https://github.com/grpc/grpc/blob/v1.62.2/setup.py#L299-L322 on the setup.py though. But I think it should be mostly fine. Same for https://github.com/grpc/grpc/blob/v1.62.2/setup.py#L53-L81.

I don't think we necessarily need to fix this right now. It could well be done in another PR (since this PR is already big enough). I think it's something we could also contribute back in the conda-forge feedstock.

Include directories are processed in left to right order, so this works as expected.
https://gcc.gnu.org/onlinedocs/gcc-6.2.0/gcc/Directory-Options.html
https://learn.microsoft.com/en-us/cpp/build/reference/i-additional-include-directories?view=msvc-170
https://clang.llvm.org/docs/ClangCommandLineReference.html#include-path-management

@cbouss cbouss merged commit 39c649b into master May 27, 2024
8 checks passed
@cbouss cbouss deleted the PKG-4724 branch May 27, 2024 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.