[libphonenumber] Adding libphonenumber port, pushing changes to get help on error #28093

trishvl · 2022-11-30T22:34:08Z

Adding port for libphonenumber OSS to work in Windows.

What does your PR fix?

Fixes issue [New Port Request] libphonenumber #27595
Which triplets are supported/not supported? Have you updated the CI baseline?

linux, No (Updated in CI baseline)
osx, No (Updated in CI baseline)
uwp, No (due to icu lib, so not updated in CI baseline)
x64-windows, Yes
x86-windows, Yes
arm64-windows, Yes
x64-windows-static, Release only (Updated in triplets)
x64-windows-static-md, Release only (Updated in triplets)
Does your PR follow the maintainer guide?

Yes. Some of the patches could be minimized and most can be added as changes upstream. Have comments on portfile to specify for help.
If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

Yes

…pushing to share code

…Debug build failing so building release only. Pushing for help.

github-actions

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

All manifest files must be formatted

./vcpkg format-manifest ports/*/vcpkg.json

Diff

diff --git a/ports/libphonenumber/vcpkg.json b/ports/libphonenumber/vcpkg.json
index d4553ba..51c7c83 100644
--- a/ports/libphonenumber/vcpkg.json
+++ b/ports/libphonenumber/vcpkg.json
@@ -3,6 +3,15 @@
   "version": "8.13.1",
   "description": "Google's common Java, C++ and JavaScript library for parsing, formatting, and validating international phone numbers.",
   "dependencies": [
+    "abseil",
+    "boost-date-time",
+    "boost-system",
+    "boost-thread",
+    "dirent",
+    "gtest",
+    "icu",
+    "protobuf",
+    "re2",
     {
       "name": "vcpkg-cmake",
       "host": true
@@ -10,15 +19,6 @@
     {
       "name": "vcpkg-cmake-config",
       "host": true
-    },
-    "boost-system",
-    "boost-thread",
-    "boost-date-time",
-    "gtest",
-    "protobuf",
-    "re2",
-    "abseil",
-    "icu",
-    "dirent"
+    }
   ]
 }

After committing all other changes, the version database must be updated

git add -u && git commit
git checkout 251741475900c9a57549d80f1b5a5e30e63d1887 -- versions
./vcpkg x-add-version --all

Diff

diff --git a/versions/baseline.json b/versions/baseline.json
index 8d8a538..87194f8 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -4092,6 +4092,10 @@
       "baseline": "2021-11-14",
       "port-version": 0
     },
+    "libphonenumber": {
+      "baseline": "8.13.1",
+      "port-version": 0
+    },
     "libplist": {
       "baseline": "1.3.6",
       "port-version": 1

You have modified or added at least one vcpkg.json where you should check the license field.

If you feel able to do so, please consider adding a "license" field to the following files:

ports/libphonenumber/vcpkg.json

Valid values for the license field can be found in the documentation

JonLiu1993 · 2022-12-05T08:19:49Z

@trishvl, Thanks for your pr, according to the ci error log, it seems that your submission file seems to be in an irregular format, please use the command ./vcpkg format-manifest ports/libphonenumber/vcpkg.json to format your vcpkg.json file

C:\a\1\s\scripts\azure-pipelines\Create-PRDiff.ps1 : The formatting of the files in the repo were not what we expected,

trishvl · 2022-12-05T18:26:14Z

@trishvl, Thanks for your pr, according to the ci error log, it seems that your submission file seems to be in an irregular format, please use the command ./vcpkg format-manifest ports/libphonenumber/vcpkg.json to format your vcpkg.json file

Just pushed changes to solve irregular format! Still running into issues with getting Debug version to work. Would a Debug build be needed for the PR?

Was also wondering if there have been previous experiences working with a ninja error: multiple rules generate issue? I headed into those errors when libphonenumbers' cmakelists would make static and shared libs under the same output name. In my commits, I just commented out the code that builds the shared libs, but wondering if there is another way I can mitigate this. Thanks!

trishvl · 2022-12-05T23:07:39Z

…in static builds, and added support for re2

github-actions

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

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

error: checked-in files for libphonenumber have changed but the version was not updated
version: 8.13.1
old SHA: 7cb9bbe59d152d21b1cefde5a0a8c02469d1cb60
new SHA: f067e35f9342a2961bb6117d7d7a6bee34be020b
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***

You have modified or added at least one vcpkg.json where you should check the license field.

If you feel able to do so, please consider adding a "license" field to the following files:

ports/libphonenumber/vcpkg.json

Valid values for the license field can be found in the documentation

JonLiu1993 · 2022-12-06T07:08:26Z

Running uwp builds, I get this error: icu[core]:x64-uwp is only supported on '!uwp' but seems like uwp builds have passed with Azure Pipelines.

I found in multiple .cpp codes upstream of icu that the header files and some variables they refer to are not applicable to UWP, so icu does not support uwp now https://github.com/unicode-org/icu/search?q=uwp

I'm not working with linux or osx so unsure how to get these running

Looks the ci x64-linux error, libphonenumber can't find icuin.lib

CMake Error at CMakeLists.txt:43 (message):
  Can't find ICU: can't locate icuin.  Please read the README.
Call Stack (most recent call first):
  CMakeLists.txt:63 (print_error)
  CMakeLists.txt:151 (find_required_library)

…triplets due to time.

trishvl · 2022-12-06T19:14:09Z

Running uwp builds, I get this error: icu[core]:x64-uwp is only supported on '!uwp' but seems like uwp builds have passed with Azure Pipelines.

I found in multiple .cpp codes upstream of icu that the header files and some variables they refer to are not applicable to UWP, so icu does not support uwp now https://github.com/unicode-org/icu/search?q=uwp

Awesome, thanks for letting me know! For now, due to time constraints with needing to work with libphonenumber via vcpkg, I'll skip linux and osx builds as well as Debug builds for x64 static triplets (getting an error with mismatched debug levels, see attached log file below).

install-x64-windows-static-dbg-out.log

trishvl · 2022-12-06T20:09:19Z

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

PRs must add only one version and must not modify any published versions
When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.
error: checked-in files for libphonenumber have changed but the version was not updated
version: 8.13.1
old SHA: 7cb9bbe59d152d21b1cefde5a0a8c02469d1cb60
new SHA: f067e35f9342a2961bb6117d7d7a6bee34be020b
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***
You have modified or added at least one vcpkg.json where you should check the license field.

If you feel able to do so, please consider adding a "license" field to the following files:

ports/libphonenumber/vcpkg.json

Valid values for the license field can be found in the documentation

Addressed this by bypassing the version check.

ras0219-msft

Thanks for the PR!

triplets/arm64-windows.cmake

ports/libphonenumber/portfile.cmake

ports/libphonenumber/fix-ninja-error-multiple-rules.patch

ports/libphonenumber/fix-absl-with-geocoder-off.patch

ras0219-msft · 2022-12-06T20:45:22Z

ports/libphonenumber/fix-windows-static-link-icu-lib.patch

+ # If ICU regexp engine is used or if the geocoder is built, use icui18n as well.
+ if (${USE_ICU_REGEXP} STREQUAL "ON" OR ${BUILD_GEOCODER} STREQUAL "ON")
+-  find_required_library (ICU_I18N unicode/regex.h icui18n "ICU")
+  find_required_library (ICU_I18N unicode/regex.h icuin "ICU")


It would be better to put this above the find_package() call above so you can add i18n to the COMPONENTS list.

Could you consider replacing find_package() here?

ports/libphonenumber/remove-shared-lib.patch

scripts/ci.baseline.txt

github-actions

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

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

error: checked-in files for libphonenumber have changed but the version was not updated
version: 8.13.1
old SHA: f067e35f9342a2961bb6117d7d7a6bee34be020b
new SHA: 48c9e00d6d04d51793c15a7d57720de09bc2c9b2
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***

You have modified or added at least one vcpkg.json where you should check the license field.

If you feel able to do so, please consider adding a "license" field to the following files:

ports/libphonenumber/vcpkg.json

Valid values for the license field can be found in the documentation

scripts/ci.baseline.txt

github-actions

You have modified or added at least one vcpkg.json where you should check the license field.

If you feel able to do so, please consider adding a "license" field to the following files:

ports/libphonenumber/vcpkg.json

Valid values for the license field can be found in the documentation

ports/libphonenumber/fix-absl-with-geocoder-off.patch

dg0yt

IMO this project needs more work to transform it into a good vcpkg port.

vcpkg wants to build the either the static lib or the shared lib.
This needs a patch to control BUILD_SHARED_LIB (which can use the standard BUILD_SHARED_LIBS), and proper passing or deriving of BUILD_STATIC_LIB.
The link libraries and usage requirements vary with linkage. For convenient use, it takes at least a proper usage file (explaining how to do find_path and find_library), or another patch to export unofficial CMake config.

ports/libphonenumber/fix-absl-with-geocoder-off.patch

ports/libphonenumber/fix-re2-identifiers.patch

github-actions

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

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

error: checked-in files for libphonenumber have changed but the version was not updated
version: 8.13.1
old SHA: 49b4ca5d52524bac7bea1d7e75ea9168d3447597
new SHA: 92a7c51466c59c74fca7f9e1232606aebe62c0e2
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***

You have modified or added at least one vcpkg.json where you should check the license field.

If you feel able to do so, please consider adding a "license" field to the following files:

ports/libphonenumber/vcpkg.json

Valid values for the license field can be found in the documentation

github-actions

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

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

error: checked-in files for libphonenumber have changed but the version was not updated
version: 8.13.1
old SHA: 49b4ca5d52524bac7bea1d7e75ea9168d3447597
new SHA: 171ec5d6a0e0e6f6241c82dee050881b2eb74608
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***

You have modified or added at least one vcpkg.json where you should check the license field.

If you feel able to do so, please consider adding a "license" field to the following files:

ports/libphonenumber/vcpkg.json

Valid values for the license field can be found in the documentation

github-actions

You have modified or added at least one vcpkg.json where you should check the license field.

If you feel able to do so, please consider adding a "license" field to the following files:

ports/libphonenumber/vcpkg.json

Valid values for the license field can be found in the documentation

ports/libphonenumber/vcpkg.json

Co-authored-by: JonLiu1993 <[email protected]>

github-actions

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

All manifest files must be formatted

./vcpkg format-manifest ports/*/vcpkg.json

Diff

diff --git a/ports/libphonenumber/vcpkg.json b/ports/libphonenumber/vcpkg.json
index 06e88f5..71d60e8 100644
--- a/ports/libphonenumber/vcpkg.json
+++ b/ports/libphonenumber/vcpkg.json
@@ -2,8 +2,8 @@
   "name": "libphonenumber",
   "version": "8.13.1",
   "description": "Google's common Java, C++ and JavaScript library for parsing, formatting, and validating international phone numbers.",
-  "supports": "!static & !linux & !osx",
   "license": "Apache-2.0",
+  "supports": "!static & !linux & !osx",
   "dependencies": [
     "abseil",
     "boost-date-time",

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

error: checked-in files for libphonenumber have changed but the version was not updated
version: 8.13.1
old SHA: 171ec5d6a0e0e6f6241c82dee050881b2eb74608
new SHA: a49f66d129392e6513eb9c622b2992f8d7c195cb
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***

github-actions

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

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

error: checked-in files for libphonenumber have changed but the version was not updated
version: 8.13.1
old SHA: 5fe1664b334b210bf2567a0dc25d96a4a7ed64b8
new SHA: a49f66d129392e6513eb9c622b2992f8d7c195cb
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***

JavierMatosD

Aside from the remaining comments, the PR looks good to me :)

JavierMatosD · 2022-12-12T23:41:18Z

ports/libphonenumber/make-test-optional.patch

+-  "test_metadata"
+-)
+-list (APPEND TESTING_LIBRARY_SOURCES "src/phonenumbers/test_metadata.cc")
+if (${BUILD_GTEST} STREQUAL "ON")


I believe the best thing to do here is to guard the testing stuff with if(FALSE). Please try to avoid shifting the code down to minimize changes :)

JavierMatosD · 2022-12-12T23:41:51Z

ports/libphonenumber/portfile.cmake

+        "fix-re2-identifiers.patch"
+        "fix-icui18n-lib-name.patch"
+        "fix-absl-with-geocoder-off.patch"
+        "make-test-optional.patch"


As per the previous comment this should probably be renamed :)

JavierMatosD · 2022-12-12T23:42:20Z

ports/libphonenumber/vcpkg.json

+    "boost-date-time",
+    "boost-system",
+    "boost-thread",
+    "gtest",


Suggested change

"gtest",

Complete

github-actions

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

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

error: checked-in files for libphonenumber have changed but the version was not updated
version: 8.13.1
old SHA: a49f66d129392e6513eb9c622b2992f8d7c195cb
new SHA: 4a8a7aedced30ba3195353fdda4103829068e299
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***

JavierMatosD · 2022-12-13T00:22:43Z

Thank you!

Adding libphonenumber port, pushing changes to get help on error

650d9ed

Adela0814 assigned JonLiu1993 Dec 1, 2022

JonLiu1993 changed the title ~~Adding libphonenumber port, pushing changes to get help on error~~ [libphonenumber] Adding libphonenumber port, pushing changes to get help on error Dec 1, 2022

JonLiu1993 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Dec 1, 2022

trishms added 2 commits December 2, 2022 10:40

Added patches to cmake file to work with vcpkg, still facing error & …

8d57c2e

…pushing to share code

Added patch to not build shared libs in order to bypass ninja error. …

e49fa01

…Debug build failing so building release only. Pushing for help.

github-actions bot requested changes Dec 5, 2022

View reviewed changes

Fixing irregular format vcpkg.json for libphonenumber

8cf9b03

Add fix to re2 dependency, and only building release

f655ab2

Fixed debugs builds by turning geocoder off, fixed icu link problems …

01fcd65

…in static builds, and added support for re2

github-actions bot requested changes Dec 6, 2022

View reviewed changes

Skipping linux and osx builds as well as debug builds for x64 static …

7810722

…triplets due to time.

trishvl marked this pull request as ready for review December 6, 2022 19:31

trishvl requested a review from github-actions[bot] December 6, 2022 19:31

ras0219-msft requested changes Dec 6, 2022

View reviewed changes

Fixing port to incorporate reviews on the PR

4cd4876

github-actions bot requested changes Dec 6, 2022

View reviewed changes

lianghaoGao reviewed Dec 6, 2022

View reviewed changes

scripts/ci.baseline.txt Outdated Show resolved Hide resolved

trishms added 2 commits December 6, 2022 15:12

Removing changes to triplets

5ab3325

Resolving validate version files by overwriting

d7f19b0

github-actions bot previously approved these changes Dec 7, 2022

View reviewed changes

JonLiu1993 added the requires:author-response label Dec 7, 2022

lianghaoGao reviewed Dec 7, 2022

View reviewed changes

ports/libphonenumber/fix-absl-with-geocoder-off.patch Show resolved Hide resolved

dg0yt reviewed Dec 8, 2022

View reviewed changes

ports/libphonenumber/fix-absl-with-geocoder-off.patch Show resolved Hide resolved

ports/libphonenumber/fix-re2-identifiers.patch Outdated Show resolved Hide resolved

github-actions bot requested changes Dec 8, 2022

View reviewed changes

Making test lib builds optional and OFF by default

ebc18f1

github-actions bot requested changes Dec 8, 2022

View reviewed changes

Resolving validate version files by overwriting

8e2e36a

github-actions bot previously approved these changes Dec 9, 2022

View reviewed changes

JonLiu1993 removed the requires:author-response label Dec 12, 2022

JonLiu1993 previously requested changes Dec 12, 2022

View reviewed changes

ports/libphonenumber/vcpkg.json Show resolved Hide resolved

JonLiu1993 added the requires:author-response label Dec 12, 2022

Update vcpkg.json to include license

dffe06b

Co-authored-by: JonLiu1993 <[email protected]>

trishvl dismissed github-actions[bot]’s stale review via dffe06b December 12, 2022 17:50

github-actions bot requested changes Dec 12, 2022

View reviewed changes

Reviewing github bot's messages

b2fca1b

github-actions bot requested changes Dec 12, 2022

View reviewed changes

Reviewing github bot's messages

5782f8c

github-actions bot previously approved these changes Dec 12, 2022

View reviewed changes

trishvl requested review from JonLiu1993 and ras0219-msft and removed request for JonLiu1993 December 12, 2022 17:59

JavierMatosD requested changes Dec 12, 2022

View reviewed changes

Making building tests false as temp solution.

0c664e0

trishvl dismissed github-actions[bot]’s stale review via 0c664e0 December 13, 2022 00:12

github-actions bot requested changes Dec 13, 2022

View reviewed changes

Reviewing github bot's messages

9bea056

github-actions bot approved these changes Dec 13, 2022

View reviewed changes

JavierMatosD approved these changes Dec 13, 2022

View reviewed changes

JavierMatosD merged commit 8e3fbf6 into microsoft:master Dec 13, 2022

FrankXie05 mentioned this pull request Dec 27, 2022

[New Port Request] libphonenumber #27595

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[libphonenumber] Adding libphonenumber port, pushing changes to get help on error #28093

[libphonenumber] Adding libphonenumber port, pushing changes to get help on error #28093

trishvl commented Nov 30, 2022 •

edited

Loading

github-actions bot left a comment

JonLiu1993 commented Dec 5, 2022

trishvl commented Dec 5, 2022

trishvl commented Dec 5, 2022 •

edited

Loading

github-actions bot left a comment

JonLiu1993 commented Dec 6, 2022

trishvl commented Dec 6, 2022

trishvl commented Dec 6, 2022

ras0219-msft left a comment

ras0219-msft Dec 6, 2022

JonLiu1993 Dec 8, 2022

github-actions bot left a comment

github-actions bot left a comment

dg0yt left a comment

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

JavierMatosD left a comment

JavierMatosD Dec 12, 2022

JavierMatosD Dec 12, 2022

JavierMatosD Dec 12, 2022

github-actions bot left a comment

JavierMatosD commented Dec 13, 2022

[libphonenumber] Adding libphonenumber port, pushing changes to get help on error #28093

[libphonenumber] Adding libphonenumber port, pushing changes to get help on error #28093

Conversation

trishvl commented Nov 30, 2022 • edited Loading

What does your PR fix?

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

Does your PR follow the maintainer guide?

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

github-actions bot left a comment

Choose a reason for hiding this comment

JonLiu1993 commented Dec 5, 2022

trishvl commented Dec 5, 2022

trishvl commented Dec 5, 2022 • edited Loading

github-actions bot left a comment

Choose a reason for hiding this comment

JonLiu1993 commented Dec 6, 2022

trishvl commented Dec 6, 2022

trishvl commented Dec 6, 2022

ras0219-msft left a comment

Choose a reason for hiding this comment

ras0219-msft Dec 6, 2022

Choose a reason for hiding this comment

JonLiu1993 Dec 8, 2022

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

dg0yt left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

JavierMatosD left a comment

Choose a reason for hiding this comment

JavierMatosD Dec 12, 2022

Choose a reason for hiding this comment

JavierMatosD Dec 12, 2022

Choose a reason for hiding this comment

JavierMatosD Dec 12, 2022

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

JavierMatosD commented Dec 13, 2022

trishvl commented Nov 30, 2022 •

edited

Loading

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

trishvl commented Dec 5, 2022 •

edited

Loading