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

[oatpp] Add new port #9402

Merged
merged 27 commits into from
May 15, 2020
Merged

[oatpp] Add new port #9402

merged 27 commits into from
May 15, 2020

Conversation

mheyman
Copy link
Contributor

@mheyman mheyman commented Dec 21, 2019

Oat++ is a "Modern Web Framework for C++. High performance, simple API, cross-platform, zero-dependency."

I managed to get the core Oat++ and most of the submodules to compile on Windows and Linux - untested on Mac. Oat++ windows support is recent and not available on the libressl submodule (requires libressl3.0 and vcpkg has 2.9.1-2) - the mbedtls submodule covers what I need so it isn't a show-stopper.

Some submodules require pkg-config to be available for building. Vcpkg doesn't seem to have support for pkg-config. Because of this, I had to go through semi-extraordinary measures to detect and use pkg-config and feel lucky that pkg-config to have found a way without resorting to patching Oat++'s CMakeLists.txt files. I did include a message about how to get pkg-config when you don't have it.

(Also, I think PR should have been a continuation of #9244 which I accidentally closed).

@JackBoosY JackBoosY self-assigned this Dec 23, 2019
@ras0219-msft
Copy link
Contributor

/azp run

ports/oatpp/CONTROL Outdated Show resolved Hide resolved
ports/oatpp/portfile.cmake Outdated Show resolved Hide resolved
ports/oatpp/portfile.cmake Outdated Show resolved Hide resolved
set(OATPP_VERSION "0.19.11")

# go to extraordinary measures to find pkg-config on windows and set the PKG_CONFIG environment variable.
function(verify_pkg_config SUBMODULE_NAME)
Copy link
Contributor

Choose a reason for hiding this comment

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

vcpkg does not depend on using pkgconfig, please implement this code in other ways (e.g. find_program / find_package / find_library).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, I'm not an Oat++ developer (just a user) - it may take me a while to figure out how to make patches for the cmake files to get rid of the pkgconfig dependency. If I get a version that works for both vcpkg and the other Oat++ targets, I'll certainly do a pull request for Oat++ to hopefully get rid of Oat++'s dependency on pkgconfig. In the mean time, I've been building Oat++ vcpkg with pkgconfig on Windows and Linux with enough success to be useful - that is, I haven't run into any debug vs release ABI issues (because I defaulted to pkgconfig using release builds).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a patch that detects if compiling in vcpkg and sets the PKG_CURL... variables appropriately for the rest of the build.

ports/oatpp/portfile.cmake Outdated Show resolved Hide resolved
@JackBoosY JackBoosY changed the title oatpp 0.19.11 port [oatpp] Add new port Dec 24, 2019
set(OATPP_VERSION "0.19.11")

vcpkg_check_linkage(ONLY_STATIC_LIBRARY)
# For when OATPP dynamic linkage builds:
Copy link
Contributor

Choose a reason for hiding this comment

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

If we don't need this code, please delete these commented codes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The OATPP_VERSION variable is required.

Oat++ doesn't build with dynamic linkage - I've started looking into the changes necessary and, while I'm no expert on C++ classes in DLLs and shared libraries, I think the changes would be pretty extensive.

Copy link
Contributor

Choose a reason for hiding this comment

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

My question is OATPP_BUILD_SHARED_LIBRARIES_OPTION is used in vcpkg_configure_cmake, but it is commented out here.

@JackBoosY
Copy link
Contributor

/azp run

@JackBoosY
Copy link
Contributor

Any progress?

Copy link
Contributor

@PhoebeHui PhoebeHui left a comment

Choose a reason for hiding this comment

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

@mheyman, thanks for your PR!

@vicroms, what do you think?

ports/oatpp/portfile.cmake Outdated Show resolved Hide resolved
ports/oatpp/portfile.cmake Outdated Show resolved Hide resolved
ports/oatpp/portfile.cmake Outdated Show resolved Hide resolved
ports/oatpp/portfile.cmake Outdated Show resolved Hide resolved
ports/oatpp/portfile.cmake Outdated Show resolved Hide resolved
ports/oatpp/portfile.cmake Outdated Show resolved Hide resolved
set(OATPP_VERSION "0.19.11")

vcpkg_check_linkage(ONLY_STATIC_LIBRARY)
# For when OATPP dynamic linkage builds:
Copy link
Contributor

Choose a reason for hiding this comment

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

My question is OATPP_BUILD_SHARED_LIBRARIES_OPTION is used in vcpkg_configure_cmake, but it is commented out here.

ports/oatpp-consul/portfile.cmake Outdated Show resolved Hide resolved
ports/oatpp-curl/portfile.cmake Outdated Show resolved Hide resolved
ports/oatpp-libressl/portfile.cmake Outdated Show resolved Hide resolved
ports/oatpp-mbedtls/portfile.cmake Outdated Show resolved Hide resolved
ports/oatpp-swagger/portfile.cmake Outdated Show resolved Hide resolved
ports/oatpp/portfile.cmake Outdated Show resolved Hide resolved
ports/oatpp-websocket/portfile.cmake Outdated Show resolved Hide resolved
@JackBoosY
Copy link
Contributor

LGTM, thanks for this PR!

@JackBoosY
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label May 15, 2020
@strega-nil
Copy link
Contributor

Alright, this looks good to me. thanks @mheyman! :D

@strega-nil strega-nil merged commit d1729dc into microsoft:master May 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants