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

[avro-cpp] Fix static build #32854

Merged
merged 2 commits into from Aug 2, 2023
Merged

Conversation

xiaozhuai
Copy link
Contributor

@xiaozhuai xiaozhuai commented Jul 31, 2023

  • Changes comply with the maintainer guide
  • SHA512s are updated for each updated download
  • The "supports" clause reflects platforms that may be fixed by this new version
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

Fix #32848

Also fixed these in ci baseline.

avro-cpp:arm-neon-android=fail
avro-cpp:arm64-android=fail
avro-cpp:x64-android=fail

@Adela0814 Adela0814 self-assigned this Jul 31, 2023
@Cheney-W Cheney-W added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Jul 31, 2023
@xiaozhuai
Copy link
Contributor Author

xiaozhuai commented Aug 1, 2023

Usage passed on x64-osx with following demo code

cmake_minimum_required(VERSION 3.15)
project(test_avro_cpp)

set(CMAKE_CXX_STANDARD 17)

find_package(unofficial-avro-cpp CONFIG REQUIRED)

add_executable(test_avro_cpp main.cpp cpx.hh)
set_source_files_properties("cpx.hh" PROPERTIES GENERATED TRUE)

find_program(ACROGENCPP avrogencpp REQUIRED)
add_custom_command(
        OUTPUT "cpx.hh"
        COMMAND
            "${ACROGENCPP}"
            "-i"
            "cpx.json"
            "-o"
            "cpx.hh"
            "-n"
            "c"
        DEPENDS
            "cpx.json"
        WORKING_DIRECTORY
            "${CMAKE_CURRENT_SOURCE_DIR}"
)

target_link_libraries(test_avro_cpp PRIVATE unofficial::avro-cpp::avrocpp)
{
  "type": "record",
  "name": "cpx",
  "fields" : [
    {"name": "re", "type": "double"},
    {"name": "im", "type" : "double"}
  ]
}
#include <iostream>
#include "cpx.hh"
#include "avro/Encoder.hh"
#include "avro/Decoder.hh"


int main() {
    std::unique_ptr<avro::OutputStream> out = avro::memoryOutputStream();
    avro::EncoderPtr e = avro::binaryEncoder();
    e->init(*out);
    c::cpx c1{};
    c1.re = 1.0;
    c1.im = 2.13;
    avro::encode(*e, c1);

    std::unique_ptr<avro::InputStream> in = avro::memoryInputStream(*out);
    avro::DecoderPtr d = avro::binaryDecoder();
    d->init(*in);

    c::cpx c2{};
    avro::decode(*d, c2);
    std::cout << '(' << c2.re << ", " << c2.im << ')' << std::endl;
    return 0;
}

Copy link
Contributor

@dg0yt dg0yt left a comment

Choose a reason for hiding this comment

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

IMO the existing usage needs to be rectified (or removed).
It doesn't consistently look for debug and release libs.
It only deals with lib acrocpp. It doesn't carry any link libraries for static linkage. (This doesn't mean that the port wouldn't support static linkage. It just means that users are required to have more knowledge about the package.)
For static linkage, the port could benefit from exporting unofficial cmake config.

ports/avro-cpp/fix-static.patch Outdated Show resolved Hide resolved
@Adela0814 Adela0814 added the requires:all-feature-testing vcpkg install port[all features supported by that port] needs to be demonstrated to function label Aug 1, 2023
@xiaozhuai
Copy link
Contributor Author

xiaozhuai commented Aug 1, 2023

@dg0yt
It should work as expected now.

Copy link
Contributor

@dg0yt dg0yt left a comment

Choose a reason for hiding this comment

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

A lot of new comments... Some of the problems are more related to the poor original state of the port/patch, not to the contribution in this PR which is a signifcant progress.

ports/avro-cpp/fix-cmake.patch Outdated Show resolved Hide resolved
ports/avro-cpp/fix-cmake.patch Outdated Show resolved Hide resolved
ports/avro-cpp/fix-cmake.patch Show resolved Hide resolved
ports/avro-cpp/fix-cmake.patch Outdated Show resolved Hide resolved
ports/avro-cpp/fix-cmake.patch Outdated Show resolved Hide resolved
ports/avro-cpp/fix-cmake.patch Outdated Show resolved Hide resolved
ports/avro-cpp/fix-cmake.patch Outdated Show resolved Hide resolved
@xiaozhuai
Copy link
Contributor Author

xiaozhuai commented Aug 1, 2023

Some of the problems are more related to the poor original state of the port/patch

@dg0yt
That's right, I'd like to write a CMakeLists from scratch rather than patch it.

@xiaozhuai
Copy link
Contributor Author

@dg0yt

Now that the patch is smaller and all your suggestions are dealt with, please review again.

@xiaozhuai xiaozhuai changed the title [avro-cpp] Fix windows static build [avro-cpp] Fix static build Aug 1, 2023
@Adela0814 Adela0814 added info:reviewed Pull Request changes follow basic guidelines and removed requires:all-feature-testing vcpkg install port[all features supported by that port] needs to be demonstrated to function labels Aug 2, 2023
@JavierMatosD JavierMatosD merged commit 4bc219e into microsoft:master Aug 2, 2023
15 checks passed
@xiaozhuai xiaozhuai deleted the dev-avro-cpp branch August 3, 2023 02:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[avro-cpp:x64-windows-static] build failure
5 participants