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

[nu-book-zxing-cpp] New port #22657

Merged
merged 55 commits into from
Feb 15, 2022
Merged

[nu-book-zxing-cpp] New port #22657

merged 55 commits into from
Feb 15, 2022

Conversation

joiskash
Copy link
Contributor

@joiskash joiskash commented Jan 20, 2022

Describe the pull request

  • What does your PR fix?

This Fixes #10503. It is a closed issue. I cannot reopen it .

  • Which triplets are supported/not supported? Have you updated the CI baseline?

    It supports all but I have tried it only on x64-windows. No I have not updated the baseline.

  • Does your PR follow the maintainer guide?

    Yes

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

    I am still working on this PR

If you are still working on the PR, open it as a Draft: https://github.blog/2019-02-14-introducing-draft-pull-requests/

@ghost
Copy link

ghost commented Jan 20, 2022

CLA assistant check
All CLA requirements met.

@JonLiu1993 JonLiu1993 self-assigned this Jan 21, 2022
@JonLiu1993 JonLiu1993 added the category:port-update The issue is with a library, which is requesting update new revision label Jan 21, 2022
@JonLiu1993 JonLiu1993 changed the title Update zxing-cpp port [zxing-cpp] Update port to 1.2.0 Jan 21, 2022
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 5ef52b5b75887fb150711f5effb221dd98b99e6f -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/z-/zxing-cpp.json b/versions/z-/zxing-cpp.json
index 627e129..cfdec8a 100644
--- a/versions/z-/zxing-cpp.json
+++ b/versions/z-/zxing-cpp.json
@@ -1,7 +1,7 @@
 {
   "versions": [
     {
-      "git-tree": "803264b5c51e6e0563a937bddc0a9d23cd399f6e",
+      "git-tree": "ed61e48b25b3d69762f58a5a3efa1c379c3b727f",
       "version-string": "2022-01",
       "port-version": 4
     },

ports/zxing-cpp/vcpkg.json Outdated Show resolved Hide resolved
…ge):

  error: could not find git for clone of fmtlib-populate.
Fix version as per feedback
@dg0yt
Copy link
Contributor

dg0yt commented Jan 21, 2022

What is it doing with git and ExternalProject? Please no hidden downloads and vendored dependencies.

@joiskash
Copy link
Contributor Author

What is it doing with git and ExternalProject? Please no hidden downloads and vendored dependencies.

https://github.com/nu-book/zxing-cpp/blob/master/test/blackbox/CMakeLists.txt

if (NOT fmt_FOUND)
    include(FetchContent)
    FetchContent_Declare (fmtlib
            GIT_REPOSITORY https://github.com/fmtlib/fmt.git
            GIT_TAG 8.0.1)
    FetchContent_MakeAvailable (fmtlib) # Adds fmt::fmt
endif()

This is not part of the core. It is part of test/blackbox we could probably just remove it

@joiskash
Copy link
Contributor Author

I tried to add fmt as a dependency in the manifest file, that did not help.

@dg0yt
Copy link
Contributor

dg0yt commented Jan 21, 2022

Thanks for the response. Apart from tests being generally skipped in vcpkg ports, it should really use vcpkg's fmt if needed.
It provides cmake config including fmt::fmt, so it must work.

Looking around, it seems that BUILD_SYSTEM_DEPS should be set to ALWAYS (actually meaning to always use system dependencies instead of vendored dependencies).
https://github.com/nu-book/zxing-cpp/blob/485a72b73fe549a623bf3cf7220c58bf6c1b1cdb/CMakeLists.txt#L11

@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Feb 5, 2022
@ras0219-msft ras0219-msft added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Feb 8, 2022
Copy link
Contributor

@ras0219-msft ras0219-msft left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

Since:

  1. This conflicts with the existing zxing-cpp port
  2. The existing port has no consumers
  3. The existing port is abandoned with multiple inquiries on its GitHub with no response from someone with commit access
  4. This new port seems to be well maintained and listed on the main zxing page

It makes sense to remove the abandoned zxing-cpp and instead build this library in CI and promote it for vcpkg users. I've pushed a commit to do that to https://github.com/ras0219-msft/vcpkg/tree/dev/roschuma/nu-book-zxing-cpp, so please run:

git pull https://github.com/ras0219-msft/vcpkg dev/roschuma/nu-book-zxing-cpp

and push the result to this PR.

We will also discuss this on Thursday PST to confirm that we want to remove the existing zxing-cpp port.

scripts/ci.baseline.txt Outdated Show resolved Hide resolved
@joiskash
Copy link
Contributor Author

Sure I will. I have not consumed any changes from your branch yet. I thought of just waiting until the decision of keeping this as a new port or replacing the old one has been made. Thank you for your comments!

@ras0219-msft
Copy link
Contributor

We have made the decision that we should add this as a new port and remove the old port.

@joiskash
Copy link
Contributor Author

I have cherry picked the commit from your branch @ras0219-msft , let me know if anything else needs to be done.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 5e98739ce23979ac0cd19e366a9f3fd08571e183 -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/baseline.json b/versions/baseline.json
index 4722611..62019f9 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -7564,6 +7564,10 @@
       "baseline": "2021-04-23",
       "port-version": 0
     },
+    "zxing-cpp": {
+      "baseline": "2020-12",
+      "port-version": 3
+    },
     "zydis": {
       "baseline": "3.2.1",
       "port-version": 0

@JackBoosY
Copy link
Contributor

@ras0219-msft Should we rename this port to zxing-cpp?

@joiskash
Copy link
Contributor Author

ENV{CL} "$ENV{CL} -wd4996" this did not seem to work. uwp build is failing due to error C4996

@ras0219-msft
Copy link
Contributor

ras0219-msft commented Feb 12, 2022

@ras0219-msft Should we rename this port to zxing-cpp?

No, while this library does conflict with the original zxing-cpp, it makes no attempt to be a drop-in replacement. Therefore it is inappropriate to have the name zxing-cpp refer to this library.

ENV{CL} "$ENV{CL} -wd4996" this did not seem to work. uwp build is failing due to error C4996

Let's mark this as unsupported on UWP for now. Unfortunately, we use MSBuild when targeting UWP which behaves very differently and would require patching to fix. This can be done by adding "supports": "!uwp" in the manifest file.

@joiskash
Copy link
Contributor Author

joiskash commented Feb 12, 2022

Let's mark this as unsupported on UWP for now. Unfortunately, we use MSBuild when targeting UWP which behaves very differently and would require patching to fix. This can be done by adding "supports": "!uwp" in the manifest file.

Ok, but passing it this way DCMAKE_CXX_FLAG=-wd4996 as an OPTION to vcpkg_cmake_configure works fine. That's what I had done previously.
Either way, Im ok with not supporting uwp.
Please have a look at the portfile just before your commit.

@JackBoosY JackBoosY added requires:author-response and removed info:reviewed Pull Request changes follow basic guidelines labels Feb 14, 2022
joiskash and others added 2 commits February 14, 2022 13:07
Co-authored-by: Jack·Boos·Yu <47264268+JackBoosY@users.noreply.github.com>
@JackBoosY JackBoosY added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. labels Feb 14, 2022
@ras0219-msft ras0219-msft merged commit 31c711c into microsoft:master Feb 15, 2022
@ras0219-msft
Copy link
Contributor

Thanks and sorry for the long journey here!

@joiskash
Copy link
Contributor Author

No problem. Im glad that my contribution got merged into main :)

ekilmer added a commit to ekilmer/vcpkg that referenced this pull request Feb 15, 2022
* master: (54 commits)
  [imgui] Update to 1.87 [implot] Update to 0.13 (microsoft#22988)
  [nu-book-zxing-cpp] New port  (microsoft#22657)
  [librabbitmq] Update to 0.11.0 (microsoft#23037)
  [doc] Add doc for `supports` expression `staticcrt` (microsoft#23079)
  [doctest] Update to 2.4.8 (microsoft#23081)
  [log4cplus] Remove unneeded catch dependency (microsoft#23066)
  [Azure SDK] Update vcpkg ports for Feb Release (microsoft#23080)
  [Freerdp] Update to 2.5.0 (microsoft#23095)
  Update vcpkg-tool to 2022-02-11 (microsoft#23059)
  Minor bugfixes to MacOS deployment readme. (microsoft#23062)
  [ci.baseline.txt] Skip colmap on osx due to metis conflict (microsoft#23047)
  [gtkmm] update to 4.6.0 (microsoft#23024)
  [faiss] Update to 1.7.2 (microsoft#22705)
  [ocilib] Disable warning C4191 (microsoft#23028)
  [polyhook2] Update to latest  (microsoft#23044)
  Add notice about how to export unofficial CMake targets. (microsoft#23041)
  [Spirv reflect] Add new port (microsoft#22295)
  [easyhook] Update target .NET Framework version to 4.7.2. (microsoft#23040)
  [gh suggestions] change license link, make it details (microsoft#22946)
  [field3d] Remove port (microsoft#22463)
  ...
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.

[New Port Request] ZXing
7 participants