Skip to content

Conversation

@dg0yt
Copy link
Contributor

@dg0yt dg0yt commented Feb 22, 2023

Fixes #29776 (regression).

  • 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.

@dg0yt
Copy link
Contributor Author

dg0yt commented Feb 22, 2023

icu-uc.pc now (x64-mingw-static):

...
Libs: "-L${libdir}" -licuuc -licudt ${baselibs}
Cflags: "-I${includedir}"

@equeim
Copy link
Contributor

equeim commented Feb 22, 2023

BTW Original icu-uc.pc also doesn't have quotes around -I${includedir} and -L${libdir}. I'm not sure whether this can be an issue but maybe vcpkg_fixup_pkgconfig just shouldn't add quotes when there weren't any in original file?

@Neumann-A
Copy link
Contributor

BTW Original icu-uc.pc also doesn't have quotes around -I${includedir} and -L${libdir}. I'm not sure whether this can be an issue but maybe vcpkg_fixup_pkgconfig just shouldn't add quotes when there weren't any in original file?

No that is requirement to support paths with spaces. An no it is generally not an issue.

@jimwang118 jimwang118 added the category:port-bug The issue is with a library, which is something the port should already support label Feb 23, 2023
@dg0yt
Copy link
Contributor Author

dg0yt commented Feb 23, 2023

BTW Original icu-uc.pc also doesn't have quotes around -I${includedir} and -L${libdir}. I'm not sure whether this can be an issue but maybe vcpkg_fixup_pkgconfig just shouldn't add quotes when there weren't any in original file?

For these flags, this has been a feature of vcpkg_fixup_pkgconfig for a while. AFAICT the quoting is meant to deal with space characters in paths. Most upstreams don't handle this case at all.

@dg0yt
Copy link
Contributor Author

dg0yt commented Feb 23, 2023

litehtml:arm64-windows failed with FILE_CONFLICTS.

We need a PR check against file lists from full CI.

@jimwang118 jimwang118 added category:scripts-audit and removed category:port-bug The issue is with a library, which is something the port should already support labels Feb 23, 2023
@dg0yt
Copy link
Contributor Author

dg0yt commented Feb 24, 2023

And now the next baseline regression, in port mnn:

FAILED: CMakeFiles/MNNMath.dir/source/math/Matrix.cpp.o 
/usr/bin/c++ -DDEBUG -DMNN_DEBUG -DMNN_DEBUG_MEMORY -DMNN_DEBUG_TENSOR_SIZE -DMNN_SUPPORT_TFLITE_QUAN -DMNN_USE_SSE -DMNN_USE_THREAD_POOL -DMNN_VERSION=\"1.1.0.0-d6795ad\" -DMNN_VERSION_MAJOR=1 -DMNN_VERSION_MINOR=1 -DMNN_VERSION_PATCH=0 -I/mnt/vcpkg-ci/buildtrees/mnn/src/1.1.0-9f04ab166e.clean/include -I/mnt/vcpkg-ci/buildtrees/mnn/src/1.1.0-9f04ab166e.clean/source -I/mnt/vcpkg-ci/buildtrees/mnn/src/1.1.0-9f04ab166e.clean/express -I/mnt/vcpkg-ci/buildtrees/mnn/src/1.1.0-9f04ab166e.clean/tools -I/mnt/vcpkg-ci/buildtrees/mnn/src/1.1.0-9f04ab166e.clean/schema/current -I/mnt/vcpkg-ci/buildtrees/mnn/src/1.1.0-9f04ab166e.clean/3rd_party/half -I/mnt/vcpkg-ci/installed/x64-linux/share/rapidjson/../../include -isystem /mnt/vcpkg-ci/installed/x64-linux/include -fPIC -std=c++11 -D__STRICT_ANSI__ -g -fstrict-aliasing -ffunction-sections -fdata-sections -ffast-math -fno-rtti -fno-exceptions  -g   -fPIC -std=gnu++11 -MD -MT CMakeFiles/MNNMath.dir/source/math/Matrix.cpp.o -MF CMakeFiles/MNNMath.dir/source/math/Matrix.cpp.o.d -o CMakeFiles/MNNMath.dir/source/math/Matrix.cpp.o -c /mnt/vcpkg-ci/buildtrees/mnn/src/1.1.0-9f04ab166e.clean/source/math/Matrix.cpp
In file included from /mnt/vcpkg-ci/installed/x64-linux/include/absl/base/config.h:86,
                 from /mnt/vcpkg-ci/installed/x64-linux/include/absl/base/attributes.h:37,
                 from /mnt/vcpkg-ci/installed/x64-linux/include/absl/strings/string_view.h:39,
                 from /mnt/vcpkg-ci/installed/x64-linux/include/flatbuffers/base.h:237,
                 from /mnt/vcpkg-ci/installed/x64-linux/include/flatbuffers/array.h:22,
                 from /mnt/vcpkg-ci/installed/x64-linux/include/flatbuffers/flatbuffers.h:24,
                 from /mnt/vcpkg-ci/buildtrees/mnn/src/1.1.0-9f04ab166e.clean/schema/current/Tensor_generated.h:7,
                 from /mnt/vcpkg-ci/buildtrees/mnn/src/1.1.0-9f04ab166e.clean/source/core/TensorUtils.hpp:13,
                 from /mnt/vcpkg-ci/buildtrees/mnn/src/1.1.0-9f04ab166e.clean/source/math/Matrix.cpp:12:
/mnt/vcpkg-ci/installed/x64-linux/include/absl/base/policy_checks.h:79:2: error: #error "C++ versions less than C++14 are not supported."
   79 | #error "C++ versions less than C++14 are not supported."
      |  ^~~~~

I was afraid that something like this would happen with #29692. CC @coryan

@coryan
Copy link
Contributor

coryan commented Feb 24, 2023

Sorry about this. It seems that flatbuffers fixed this upstream:

google/flatbuffers#7824

Could we apply the same patch here?

Mostly so I can explain to myself what happened here: it is a bit of a mess, the flatbuffers port in vcpkg does not depend on Abseil (flatbuffers compiles fine without it). So it was not recompiled / tested when I updated Abseil. However, flatbuffers will try to use Abseil if it is present, for example, if it has been installed by another port that depends on Abseil and on flatbuffers.

BTW, this suggests the flatbuffers port does not have a stable ABI, even after installed. If I install the flatbuffers port I get one definition of this class:

https://github.com/google/flatbuffers/blob/f7a75173f171b5edae59a1ff28e5b408057e60ac/include/flatbuffers/string.h#L30-L34

But if I later I install Abseil then I get a different definition, because now the header may be able to find absl/strings/string_view.h.

/FYI: @devjgm

@coryan
Copy link
Contributor

coryan commented Mar 2, 2023

As you may have noticed I just sent #29974. Hopefully that will merged soon and unblock your work here.

@Cheney-W Cheney-W added category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly and removed category:scripts-audit labels Mar 3, 2023
@dg0yt
Copy link
Contributor Author

dg0yt commented Mar 12, 2023

Ping @Cheney-W @jimwang118 for review.

@jimwang118 jimwang118 requested a review from Cheney-W March 13, 2023 03:09
@Cheney-W Cheney-W added the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label Mar 13, 2023
@dan-shaw dan-shaw merged commit 5da7b20 into microsoft:master Mar 15, 2023
@dg0yt dg0yt deleted the pkgconfig-fixup branch March 15, 2023 05:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[icu] Link error when linking against ICU on Windows using pkgconfig

7 participants