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

[blpapi] add new port #32603

Merged
merged 2 commits into from
Jul 25, 2023
Merged

[blpapi] add new port #32603

merged 2 commits into from
Jul 25, 2023

Conversation

sweemer
Copy link
Contributor

@sweemer sweemer commented Jul 17, 2023

  • Changes comply with the maintainer guide
  • The name of the port matches an existing name for this component on https://repology.org/ if possible, and/or is strongly associated with that component on search engines.
  • Optional dependencies are resolved in exactly one way. For example, if the component is built with CMake, all find_package calls are REQUIRED, are satisfied by vcpkg.json's declared dependencies, or disabled with CMAKE_DISABLE_FIND_PACKAGE_Xxx
  • The versioning scheme in vcpkg.json matches what upstream says.
  • The license declaration in vcpkg.json matches what upstream says.
  • The installed as the "copyright" file matches what upstream says.
  • The source code of the component installed comes from an authoritative source.
  • The generated "usage text" is accurate. See adding-usage for context.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is in the new port's versions file.
  • Only one version is added to each modified port's versions file.

Disclaimer: I am not affiliated with Bloomberg in any way. I am simply adding this port based on their publicly available libraries.

I have two questions:

  1. Their license looks like MIT but I don't think I should be the one to say for sure. Can we leave the license declaration blank until we can get somebody from Bloomberg to clarify?
  2. Bloomberg only provide dynamic libraries, so I have reflected that in the supports field; however, I would like to remove that restriction and force dynamic linkage even when the user is using a static triplet. Can you guide me on how to do that?

@sweemer sweemer marked this pull request as draft July 18, 2023 00:25
@JonLiu1993 JonLiu1993 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Jul 18, 2023
@sweemer sweemer force-pushed the blpapi branch 2 times, most recently from 60b64a1 to dba4a2c Compare July 18, 2023 11:28
@sweemer sweemer marked this pull request as ready for review July 18, 2023 11:39
Comment on lines 7 to 16
"dependencies": [
{
"name": "vcpkg-cmake",
"host": true
},
{
"name": "vcpkg-cmake-config",
"host": true
}
]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"dependencies": [
{
"name": "vcpkg-cmake",
"host": true
},
{
"name": "vcpkg-cmake-config",
"host": true
}
]

Copy link
Member

Choose a reason for hiding this comment

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

@sweemer, Thanks for your PR, vcpkg_cmake_configure and vcpkg_cmake_install provided by vcpkg-cmake port
vcpkg_cmake_config_fixup provided by vcpkg-cmake-config port, blpapi does not use these functions, so "vcpkg-cmake" and "vcpkg-cmake-config" should not be used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@JonLiu1993
Copy link
Member

Tested usage successfully by blpapi:x64-windows:

The package blpapi provides CMake targets:

    find_package(blpapi CONFIG REQUIRED)
    target_link_libraries(main PRIVATE blpapi)

@JonLiu1993
Copy link
Member

Note: I will be converting your PR to draft status. When you respond, please revert to "ready for review". That way, I can be aware that you've responded since you can't modify the tags.

@sweemer sweemer marked this pull request as ready for review July 20, 2023 11:45
JonLiu1993
JonLiu1993 previously approved these changes Jul 21, 2023
@JonLiu1993 JonLiu1993 added the info:reviewed Pull Request changes follow basic guidelines label Jul 21, 2023
if (VCPKG_TARGET_IS_LINUX)
file(GLOB SO_FILES LIST_DIRECTORIES false "${SOURCE_PATH}/Linux/*${BITS_SUFFIX}.so")
else()
file(GLOB DLL_FILES LIST_DIRECTORIES false "${SOURCE_PATH}/lib/*${BITS_SUFFIX}.dll")
Copy link
Member

Choose a reason for hiding this comment

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

I checked that the DLLs don't appear to link with a CRT so this debug release mismatch might be OK.

@BillyONeal
Copy link
Member

Their license looks like MIT but I don't think I should be the one to say for sure. Can we leave the license declaration blank until we can get somebody from Bloomberg to clarify?

LGTM

Bloomberg only provide dynamic libraries, so I have reflected that in the supports field; however, I would like to remove that restriction and force dynamic linkage even when the user is using a static triplet. Can you guide me on how to do that?

It would literally just be removing that block from the supports clause. The problem is that dynamic linkage with static CRT is usually doom so we intentionally don't make that easy to do out of the box.

@BillyONeal BillyONeal removed the info:reviewed Pull Request changes follow basic guidelines label Jul 21, 2023
@BillyONeal BillyONeal marked this pull request as draft July 21, 2023 20:50
@BillyONeal
Copy link
Member

Please double check that you're happy with nitpicks I fixed, and if so, indicate that you are happy by marking this 'Ready for review'

@sweemer
Copy link
Contributor Author

sweemer commented Jul 23, 2023

@BillyONeal I'm happy with the changes you've made.

Regarding forced dynamic linkage, do I understand correctly that you don't recommend forcing it with a static triplet? If so, then I will leave it the way it is. But if it's acceptable in this case then I would like to remove that part of the supports clause before merging. If the concern is about dynamic linkage with static CRT then I don't mind changing it to something like (linux | (windows & !uwp)) & !(!static & staticcrt) & (x86 | x64).

@sweemer sweemer marked this pull request as ready for review July 23, 2023 04:50
@BillyONeal
Copy link
Member

Regarding forced dynamic linkage, do I understand correctly that you don't recommend forcing it with a static triplet?

You can force dynamic linkage with a static triplet if the resulting DLL doesn't speak about the standard library at all. Most libraries aren't prepared to deal with, for example, std::vector&s that you can't use because they belong to a totally different CRT.

There are global variables like the debug lock or the locale lock which cause doom if you try to touch standard library components visibly across such an interface. When you dynamically link with the CRT, those global variables live in the CRT's DLLs, but a static triplet requests a static CRT. Static CRT plus dynamic linkage means each DLL has its own CRT, and thus its own copy of those globals, and thus can't contain standard library types in their interface at all.

@BillyONeal BillyONeal merged commit 250209e into microsoft:master Jul 25, 2023
@sweemer sweemer deleted the blpapi branch July 25, 2023 22:35
@JonLiu1993 JonLiu1993 added the info:reviewed Pull Request changes follow basic guidelines label Jul 26, 2023
@apollomok
Copy link

apollomok commented Jul 23, 2024

Is the path out-dated? vcpkg cannot download from the path.

error: https://bcms.bloomberg.com/BLPAPI-Generic/blpapi_cpp_3.20.2.2-windows.zip: WinHttpSendRequest failed with exit code 12007

@sweemer
Copy link
Contributor Author

sweemer commented Jul 23, 2024

Yes, it might need to be updated. I can take a look next week.

@sweemer
Copy link
Contributor Author

sweemer commented Jul 27, 2024

Updated in #40130

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:new-port The issue is requesting a new library to be added; consider making a PR! info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants