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

1.46.x with zlib & abseil as private #213

Merged
merged 7 commits into from
Aug 28, 2022

Conversation

hmaarrfk
Copy link
Contributor

@hmaarrfk hmaarrfk commented Aug 15, 2022

Closes conda-forge/zlib-feedstock#65
Closes #214

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I was trying to look for recipes to lint for you, but it appears we have a merge conflict.
Please try to merge or rebase with the base branch to resolve this conflict.

Please ping the 'conda-forge/core' team (using the @ notation in a comment) if you believe this is a bug.

@hmaarrfk hmaarrfk changed the base branch from main to 1.46.x August 15, 2022 18:26
@hmaarrfk hmaarrfk closed this Aug 15, 2022
@hmaarrfk hmaarrfk reopened this Aug 15, 2022
@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@h-vetinari
Copy link
Member

WTH, where is appveyor coming from now? This has been rerendered just a few days ago...

PS. Thanks a lot for fixing this!

@hmaarrfk
Copy link
Contributor Author

i accidentally targetted the main branch.

@hmaarrfk
Copy link
Contributor Author

@conda-forge-admin please rerender

@conda-forge-admin please restart cis

h-vetinari
h-vetinari previously approved these changes Aug 15, 2022
recipe/meta.yaml Outdated Show resolved Hide resolved
@hmaarrfk
Copy link
Contributor Author

I think the patch was too small, we must specify it for all the libraries:

CMake Error at CMakeLists.txt:2159 (target_link_libraries):
  The keyword signature for target_link_libraries has already been used with
  the target "grpc".  All uses of target_link_libraries with a target must be
  either all-keyword or all-plain.

  The uses of the keyword signature are here:

   * CMakeLists.txt:2139 (target_link_libraries)



CMake Error at CMakeLists.txt:2740 (target_link_libraries):
  The keyword signature for target_link_libraries has already been used with
  the target "grpc_unsecure".  All uses of target_link_libraries with a
  target must be either all-keyword or all-plain.

  The uses of the keyword signature are here:

   * CMakeLists.txt:2721 (target_link_libraries)

@h-vetinari
Copy link
Member

Yeah... Though interestingly, linux doesn't care...

@h-vetinari
Copy link
Member

h-vetinari commented Aug 15, 2022

Same issue affects abseil, it seems (non-private targets)

@h-vetinari
Copy link
Member

Same issue affects abseil, it seems (non-private targets)

I believe we should be able to remove https://github.com/grpc/grpc/blob/v1.46.4/cmake/abseil-cpp.cmake#L33-L35 on windows, since we're now building statically here...? WDYT @hmaarrfk?

@h-vetinari
Copy link
Member

In the meantime I'll open a quickfix PR to add the required run-deps (& some tests), so we can continue iterating to fix this properly, while unblocking the migrator.

@h-vetinari
Copy link
Member

OK, new patch looks cool, hoping this works now 🤞

Happy to drop #215 of course, though if that PR works, I think we should take over b06a198 at least.

@hmaarrfk
Copy link
Contributor Author

yeah, the new patch is like a sledgehammer. i'm actually woried about it. but it seems like they aren't creating many internal libraries.

@h-vetinari h-vetinari force-pushed the 1.46.x_with-zlib_private branch 2 times, most recently from 6fab016 to c5e1a74 Compare August 23, 2022 10:18
@hmaarrfk
Copy link
Contributor Author

This kind of request is typically well taken upstream.

It is also not so urgent for conda-forge

Can we make sure to communicate our ideas to them so we can find a good long term solution?

@h-vetinari
Copy link
Member

This kind of request is typically well taken upstream.

Didn't you say before that upstream doesn't like this kind of churn?

And I'm starting to see why. Their default is to use vendored everything (so the linkage isn't so important because you're bringing all targets anyway), but more importantly, the linkage for abseil cannot easily be made private because (IIUC) abseil itself doesn't package all its internal symbols correctly (OTOH, grpc explicitly also uses abseil-internal targets).

So it becomes hard to come up with a general solution.

It is also not so urgent for conda-forge

I tend to disagree - this is blocking the abseil migration & further grpc stuff. Thankfully, it seems I'm converging to a solution here.

Can we make sure to communicate our ideas to them so we can find a good long term solution?

That we can do. I'll look at opening an issue.

also: don't make abseil private on unix
h-vetinari pushed a commit to h-vetinari/grpc-cpp-feedstock that referenced this pull request Aug 23, 2022
@h-vetinari
Copy link
Member

@hmaarrfk @xhochy @isuruf
I have passing builds for 1.46-1.48 that remove the public linkage of zlib (all platforms) and abseil (on windows, where we're building against the static library that doesn't have a run-export).

I'd appreciate your review or opinion.

If there are no comments, I'm intending to merge this soonish (1-2 days), because this is blocking the abseil migration and a bunch of follow-up work.

@hmaarrfk
Copy link
Contributor Author

Didn't you say conda-forge/zlib-feedstock#65 (comment) that upstream doesn't like this kind of churn?

Haha, that is probably true. They don't like it. But they are also humans and understand the need for different system confiugrations.

Will look at the rest of the stuff this weekend.

@h-vetinari
Copy link
Member

Will look at the rest of the stuff this weekend.

Do you still have time to have a look this weekend? Otherwise I'd like to merge this before the protobuf migrators

@hmaarrfk
Copy link
Contributor Author

please merge at will

@h-vetinari h-vetinari merged commit 1d65621 into conda-forge:1.46.x Aug 28, 2022
@h-vetinari
Copy link
Member

Could it be that grpc actually run-depends on zlib now?

I see errors with zlib symbols in conda-forge/bear-feedstock#22, but only in cross compilation (because native builds might be picking up zlib from the build env, which runs into arch differences for cross-compilation...?)

@h-vetinari
Copy link
Member

Yeah, seems this was not extensive enough also for abseil...

CMake Error at D:/bld/cpp-opentelemetry-sdk_1661698775065/_h_env/Library/lib/cmake/grpc/gRPCTargets.cmake:77 (set_target_properties):
  The link interface of target "gRPC::grpc" contains:

    absl::flat_hash_map

  but the target was not found.

See conda-forge/cpp-opentelemetry-sdk-feedstock#26

@h-vetinari
Copy link
Member

On the other hand, at least conda-forge/googleapis-cpp-feedstock#35 seems to work now

@h-vetinari
Copy link
Member

Yeah, seems this was not extensive enough also for abseil...

Based on looking at https://github.com/open-telemetry/opentelemetry-cpp/blob/main/CMakeLists.txt and the logs, it seems that CMake only evaluates the link interface after configuring, so the test I added is not complete because it would need to start building/installing something.

@h-vetinari
Copy link
Member

Can we make sure to communicate our ideas to them so we can find a good long term solution?

That we can do. I'll look at opening an issue.

Done: grpc/grpc#30838

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.

Something broken with zlib vs. libzlib files (or run-export)
3 participants