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

zxing-cpp: fix two issues with the recent inclusion of 2.0 #15357

Merged
merged 6 commits into from
Feb 2, 2023

Conversation

axxel
Copy link
Contributor

@axxel axxel commented Jan 19, 2023

Specify library name and version: zxing-cpp/2.0

@prince-chrismc requested this PR

I'm the maintainer of the zxing-cpp project. I'm not a conan users and have no infrastructure to test those changes.


@CLAassistant
Copy link

CLAassistant commented Jan 19, 2023

CLA assistant check
All committers have signed the CLA.

@conan-center-bot

This comment has been minimized.

prince-chrismc
prince-chrismc previously approved these changes Jan 19, 2023
@prince-chrismc
Copy link
Contributor

Make sure to comment int #4

@axxel
Copy link
Contributor Author

axxel commented Jan 20, 2023

Make sure to comment int #4

I did and my name has been added (#15375). Trying to look at the details (still) gives me a 403. What is next? Just waiting for 2 more reviews?

jwillikers
jwillikers previously approved these changes Jan 20, 2023
@prince-chrismc
Copy link
Contributor

Let me kick the bot :)

@prince-chrismc
Copy link
Contributor

There a backend (we call is c3i) and it loops over all the open PRs and since there's a lot it can take a while.

Review process and reviews are docs here https://github.com/conan-io/conan-center-index/blob/master/docs/review_process.md#rule-of-2-reviews :)

@conan-center-bot

This comment has been minimized.

@axxel
Copy link
Contributor Author

axxel commented Jan 20, 2023

The build fails foremost because the test project is build with cxx_std_14 but starting from 2.0 the client code needs to be compiled with a c++17 conforming compiler. That said, it seems some configurations did actually build and test OK, which I don't understand with my total lack of knowledge about the conan infrastructure.

I had a quick look at the different test sources and wonder what their purpose is. Why not use the ctests that come with the library? Or is the idea to check that a client code can actually be built from the installed library (which is actually a good idea, since we don't test that properly)? Why only test the writer part of the library?

Note: there is a deprecation warning about ZXing::TextUtfEncoding::FromUtf8. That function can simply be removed for 2.0, there is now an encode(std::string) overload.

Anyway: I don't have the time to first get sufficiently knowledgeable with your project to properly fix this the way you do things around here (e.g. simply build all tests with c++17 or add a new config just for version 2.0). So either someone tells me explicitly how you'd like this resolved (like sending me a diff), then I can update my PR or someone else needs to start from scratch.

@toge
Copy link
Contributor

toge commented Feb 2, 2023

@axxel
Sorry for the delay in responding.
I missed your PR and assignment.

It's my mistake.
Sorry for the inconvenience.

In order to get cci's CI to work, there are several fixes that need to be made.
As far as I have confirmed in my local environment, test_package should work with the following modifications.

test_package/CMakeLists.txt

project(test_package LANGUAGES CXX)

find_package(ZXing REQUIRED CONFIG)
find_package(stb REQUIRED CONFIG)

if (ZXing_VERSION VERSION_LESS "1.1.0")
    add_executable(${PROJECT_NAME} test_package.cpp)
elseif (ZXing_VERSION VERSION_LESS "2.0.0")
    add_executable(${PROJECT_NAME} test_package_1.1.cpp)
else()
    add_executable(${PROJECT_NAME} test_package_2.0.cpp)
endif()
target_link_libraries(${PROJECT_NAME} PRIVATE ZXing::ZXing stb::stb)

if (ZXing_VERSION VERSION_LESS "2.0.0")
    target_compile_features(${PROJECT_NAME} PRIVATE cxx_std_14)
else()
    target_compile_features(${PROJECT_NAME} PRIVATE cxx_std_17)
endif()

test_package/test_package_2.0.cpp

#include "ZXing/BarcodeFormat.h"
#include "ZXing/BitMatrix.h"
#include "ZXing/BitMatrixIO.h"
#include "ZXing/CharacterSet.h"
#include "ZXing/MultiFormatWriter.h"
#include "ZXing/TextUtfEncoding.h"

#include <algorithm>
#include <cctype>
#include <cstring>
#include <fstream>
#include <iostream>
#include <string>

#define STB_IMAGE_WRITE_IMPLEMENTATION
#include <stb_image_write.h>

using namespace ZXing;

int main(int argc, char* argv[])
{
    int width = 100, height = 100;
    int margin = 10;
    int eccLevel = 8;
    auto encoding = CharacterSet::Unknown;
    auto format = BarcodeFormat::QRCode;
    std::string text = "Hello conan world!";
    std::string outPath = "output.png";

    try {
        auto writer = MultiFormatWriter(format).setMargin(margin).setEncoding(encoding).setEccLevel(eccLevel);
        auto matrix = writer.encode(text, width, height);
        auto bitmap = ToMatrix<uint8_t>(matrix);

        int success = stbi_write_png(outPath.c_str(), bitmap.width(), bitmap.height(), 1, bitmap.data(), 0);
        if (!success) {
            std::cerr << "Failed to write image: " << outPath << std::endl;
            return -1;
        }
    } catch (const std::exception& e) {
        std::cerr << e.what() << std::endl;
        return -1;
    }

    return 0;
}

Thanks to @toge for providing the code.
@axxel axxel dismissed stale reviews from jwillikers and prince-chrismc via 592c967 February 2, 2023 08:12
Thanks to @Togo for providing the text.

Note: I committed this via the github web interface, where I did not find a way to modify two files inside one commit.
@conan-center-bot

This comment has been minimized.

@toge's suggestion did drop that line. adding it back in.
@axxel
Copy link
Contributor Author

axxel commented Feb 2, 2023

@toge Thanks for picking this up and providing me with the code to fix the build issues. As you can see there was one line missing in CMakeLists.txt I added this back in with my last commit but the jenkins build bot says "This commit cannot be built". The "Details" are still not accessible to me. So I don't know what the issue is this time.

@toge
Copy link
Contributor

toge commented Feb 2, 2023

@axxel
Sorry for the missing lines in CMakeLists.txt.
In most cases of "This commit cannot be built", a reopen (close, wait 10 seconds and open) will solve the problem.

I wish I could do it for you, but I don't have the right.
Could you please do that?

@axxel axxel closed this Feb 2, 2023
@axxel axxel reopened this Feb 2, 2023
@conan-center-bot

This comment has been minimized.

Replace all mentions of the old 'nu-book' with the new 'zxing-cpp' name in the github links.
Overlooked that in my first commit, unfortunately.
@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ✔️

All green in build 8 (c3b0a3accf62cc7bb7c661776d4936e2d3312152):

  • zxing-cpp/2.0.0@:
    All packages built successfully! (All logs)

  • zxing-cpp/1.4.0@:
    All packages built successfully! (All logs)

  • zxing-cpp/1.3.0@:
    All packages built successfully! (All logs)

  • zxing-cpp/1.0.8@:
    All packages built successfully! (All logs)


Conan v2 pipeline (informative, not required for merge) ✔️

Note: Conan v2 builds are informative and they are not required for the PR to be merged.

All green in build 7 (c3b0a3accf62cc7bb7c661776d4936e2d3312152):

  • zxing-cpp/2.0.0@:
    All packages built successfully! (All logs)

  • zxing-cpp/1.4.0@:
    All packages built successfully! (All logs)

  • zxing-cpp/1.0.8@:
    All packages built successfully! (All logs)

  • zxing-cpp/1.3.0@:
    All packages built successfully! (All logs)

@axxel axxel requested review from prince-chrismc and jwillikers and removed request for prince-chrismc February 2, 2023 23:21
@conan-center-bot conan-center-bot merged commit d23f5b9 into conan-io:master Feb 2, 2023
@axxel axxel deleted the patch-1 branch February 2, 2023 23:30
sabelka pushed a commit to sabelka/conan-center-index that referenced this pull request Feb 12, 2023
…of 2.0

* zxing-cpp: actually download 2.0.0 tarball

* zxing-cpp: update homepage information

* zxing-cpp: add testing code for version 2.0

Thanks to @toge for providing the code.

* zxing-cpp: add test_package_2.0 to CMake Lists

Thanks to @Togo for providing the text.

Note: I committed this via the github web interface, where I did not find a way to modify two files inside one commit.

* zxing-cpp: add back missing cmake_minimum_required

@toge's suggestion did drop that line. adding it back in.

* zxing-cpp: nu-book -> zxing-cpp in conandata.yml

Replace all mentions of the old 'nu-book' with the new 'zxing-cpp' name in the github links.
Overlooked that in my first commit, unfortunately.
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.

6 participants