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

Make libprotobuf a build only dependency #74

Merged
merged 19 commits into from
Oct 12, 2023

Conversation

charlesbluca
Copy link
Member

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-webservices
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.

@charlesbluca
Copy link
Member Author

@conda-forge-admin, please rerender

@charlesbluca
Copy link
Member Author

@conda-forge-admin, please rerender

@github-actions
Copy link
Contributor

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you but ran into some issues. Please check the output logs of the latest rerendering GitHub actions workflow run for errors. You can also ping conda-forge/core for further assistance or try re-rendering locally.

This message was generated by GitHub actions workflow run https://github.com/conda-forge/dask-sql-feedstock/actions/runs/6497409473.

@jakirkham
Copy link
Member

If the re-render didn't do anything, this change isn't working. Would suggesting taking a closer look

@charlesbluca
Copy link
Member Author

@conda-forge-admin, please rerender

@github-actions
Copy link
Contributor

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like there was nothing to do.

This message was generated by GitHub actions workflow run https://github.com/conda-forge/dask-sql-feedstock/actions/runs/6497631962.

@jakirkham
Copy link
Member

Might want to look at extended_keys

@bdice
Copy link
Contributor

bdice commented Oct 12, 2023

Might want to look at extended_keys

@jakirkham I looked over extended_keys but I'm not sure if I understand how it would be used here. libprotobuf is already part of a zip_keys with libgrpc, which seems like it might act differently than the example in the docs which is only one key. I pushed some commits that seem to give the right rerendering, but I'm very open to alternatives if this isn't the best approach.

… conda-smithy 3.27.1, and conda-forge-pinning 2023.10.12.13.22.36
@charlesbluca
Copy link
Member Author

@conda-forge-admin, please rerender

- 1.56
libprotobuf:
- 4.24.3
- 4.23.3
Copy link
Member

Choose a reason for hiding this comment

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

Should we keep the global pinning for libprotobuf if we are overriding libprotobuf here?

Suggested change
- 4.23.3
- 4.23.4
- 4.23.3

Copy link
Contributor

Choose a reason for hiding this comment

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

The entries in this matrix are:

  • Global pinning (4.23.4) plus migrations (which make it 4.24.3)
  • Older libprotobuf pinning that we need (4.23.3)

# replace migrators for libprotobuf 1.57 / 1.58.
libgrpc:
- 1.58
- 1.56
Copy link
Member

Choose a reason for hiding this comment

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

Also libgrpc is pinned globally at 1.57. Do we want to include this as well?

Suggested change
- 1.56
- 1.57
- 1.56

Copy link
Contributor

Choose a reason for hiding this comment

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

The entries in this matrix are:

  • Global pinning (1.57) plus migrations (which make it 1.58)
  • libgrpc matching the older libprotobuf pinning that we need

Copy link
Member

Choose a reason for hiding this comment

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

You'll need to zip and match libprotobuf with libgrpc and also libabseil; they're not interrelated on source-level, but since we now need to migrate even for protobuf patch versions, we cannot feasibly build all combinations, and so we coupled them 1:1 as follows:

abseil grpc protobuf
20230125 1.54 3.21
20230125 1.56 4.23.3
20230802 1.57 4.23.4
20230802 1.58 4.24.3

Copy link
Member

Choose a reason for hiding this comment

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

Though I guess it would be conceivable to rebuild grpc 1.56 & protobuf 4.23.3 with newer abseil.

Copy link
Member

Choose a reason for hiding this comment

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

More details here

Copy link
Member

Choose a reason for hiding this comment

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

Thanks Axel! 🙏

Currently we are hoping none of this is needed and that it was an oversight libprotobuf was in host. So are now trying to add it to build only

Though it is possible that won't work, in which case we will go back to adding in another set of versions. We had a working solution later in the commits based on a custom migrator, which was more ergonomic. This is probably what we would go back to if we need to add these in version pinnings in

@jakirkham
Copy link
Member

Might want to look at extended_keys

@jakirkham I looked over extended_keys but I'm not sure if I understand how it would be used here. libprotobuf is already part of a zip_keys with libgrpc, which seems like it might act differently than the example in the docs which is only one key. I pushed some commits that seem to give the right rerendering, but I'm very open to alternatives if this isn't the best approach.

Am worried that what we are doing here is a bit fragile since we are overriding global pinnings and manually adding values from migrators

Think it would be better if we used some method to append our values to the keys (instead of overriding them). Either using something from conda-build or by manually adding a migrator of our own to add the missed values

@jakirkham
Copy link
Member

In terms of a migrator (which may wind up being the best/simplest solution), would try...

  1. Copying one of the existing migrators to a new file
  2. Adding use_local: true
  3. Tweaking it to match whatever versions we are wanting to add
  4. Re-rendering to apply the changes

This should add new builds with the expected versions (if not repeat 3-4)

@bdice
Copy link
Contributor

bdice commented Oct 12, 2023

In terms of a migrator (which may wind up being the best/simplest solution), would try...

1. Copying one of the existing migrators to a new file

2. Adding [`use_local: true`](https://github.com/conda-forge/conda-forge-pinning-feedstock/pull/4400/commits/8c5323d2a65a49be297c216a9b4b3cd0135a3776)

3. Tweaking it to match whatever versions we are wanting to add

4. Re-rendering to apply the changes

This should add new builds with the expected versions (if not repeat 3-4)

This is a good idea. I like it!

@jakirkham
Copy link
Member

jakirkham commented Oct 12, 2023

May also need to make this change (in requirements/build):

-   - libprotobuf                            # [build_platform != target_platform]
+   - libprotobuf

@jakirkham
Copy link
Member

@conda-forge-admin , please re-render

conda-forge-webservices[bot] and others added 2 commits October 12, 2023 22:17
@jakirkham jakirkham changed the title Rebuild for libprotobuf 4.23.3 Make libprotobuf a build only dependency Oct 12, 2023
@charlesbluca charlesbluca merged commit ce6c8b8 into conda-forge:main Oct 12, 2023
22 checks passed
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.

None yet

5 participants