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

V3.21.0 #42

Closed
wants to merge 17 commits into from
Closed

V3.21.0 #42

wants to merge 17 commits into from

Conversation

adament
Copy link
Contributor

@adament adament commented Aug 9, 2023

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.

I am sorry for opening multiple pull requests, I had problems rerendering so I tried deleting my fork and reforking.

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

I do have some suggestions for making it better though...

For recipe:

  • License is not an SPDX identifier (or a custom LicenseRef) nor an SPDX license expression.

Documentation on acceptable licenses can be found here.

@adament
Copy link
Contributor Author

adament commented Aug 9, 2023

@conda-forge-admin, please rerende

@adament
Copy link
Contributor Author

adament commented Aug 9, 2023

@conda-forge-admin, please rerender

conda-forge-webservices[bot] and others added 2 commits August 9, 2023 14:40
@conda-forge-webservices
Copy link

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

I wanted to let you know that I linted all conda-recipes in your PR (recipe) and found some lint.

Here's what I've got...

For recipe:

  • Selectors are suggested to take a <two spaces>#<one space>[<expression>] form. See lines [12]

For recipe:

  • License is not an SPDX identifier (or a custom LicenseRef) nor an SPDX license expression.

Documentation on acceptable licenses can be found here.

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

I do have some suggestions for making it better though...

For recipe:

  • License is not an SPDX identifier (or a custom LicenseRef) nor an SPDX license expression.

Documentation on acceptable licenses can be found here.

The C++ library is built for MacOS X 12.0 and despite what the file name
of the python wheel indicates, the C++ library in the wheel is also
built for 12.0. So we might as well build the python module from source.
@adament
Copy link
Contributor Author

adament commented Aug 10, 2023

@conda-forge-admin, please rerender

@adament
Copy link
Contributor Author

adament commented Aug 10, 2023

@conda-forge/core This package links against a binary only C++ library from bloomberg and if I understand the error messages correctly, this library is built for MacOS X 12.0. Hence the package build fails on OS X when we build for 10.9. Bloomberg also provides binary wheels for macos x, which the filename blpapi-3.21.0-cp39-cp39-macosx_10_9_x86_64.whl indicates should run on 10.9, but when I try to build the package from the wheel directly on OS X, the import test still gives an error that the bundled libblpapi3_64.so is built for MacOS X 12.0. This leads me to believe that the filename of the wheel is incorrect. Hence I tried building the python module from source on all platforms and bumping the Mac OS X dependency to 12.0. But then the build errors out because the Mac OS X 12.0 SDK is not available from phracker.

Should I drop OS X support on this package or can we build against the 12.0 SDK?

@adament adament marked this pull request as ready for review August 10, 2023 07:12
@ocefpaf
Copy link
Member

ocefpaf commented Aug 10, 2023

Should I drop OS X support on this package or can we build against the 12.0 SDK?

You can try a different SDK before dropping it:

https://conda-forge.org/docs/maintainer/knowledge_base.html#requiring-newer-macos-sdks

@adament
Copy link
Contributor Author

adament commented Aug 10, 2023

Thank you, do you mean I should try to bisect the oldest SDK the build will accept, even if the C++ library was built against the 12.0 SDK?

@adament
Copy link
Contributor Author

adament commented Aug 10, 2023

@conda-forge-admin, please rerender

conda-forge-webservices[bot] and others added 2 commits August 10, 2023 12:45
@adament
Copy link
Contributor Author

adament commented Aug 10, 2023

@conda-forge-admin, please rerender

@ocefpaf
Copy link
Member

ocefpaf commented Aug 10, 2023

Thank you, do you mean I should try to bisect the oldest SDK the build will accept, even if the C++ library was built against the 12.0 SDK?

No idea. I never even held a mac on my hands. I'd try the same SDK and, if that doesn't work and the @conda-forge/blpapi team doesn't care about osx, I'd just drop it and open an issue to follow up later when someone with the expertise shows up.

@adament
Copy link
Contributor Author

adament commented Aug 10, 2023

No idea. I never even held a mac on my hands. I'd try the same SDK and, if that doesn't work and the @conda-forge/blpapi team doesn't care about osx, I'd just drop it and open an issue to follow up later when someone with the expertise shows up.

Ah thank you, then we are in similar situations with regard to macs.

I just tried with the 11.0 and 11.3 SDKs and they still fail upon import with a missing symbol error and a note about how the C++ library was built against the 12.0 SDK. And let me just reiterate that attempting to build against the 12.0 SDK failed because newer SDKs than 11.3 are not available in the
phracker/MacOSX-SDKs#44 repository which the conda-forge pipeline infrastructure seems to rely on.

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.

2 participants