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.

Sign up for GitHub

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

Jump to bottom

[daw-json-link] New port #18411

Merged
merged 147 commits into from
Aug 4, 2021
Merged

[daw-json-link] New port #18411

merged 147 commits into from
Aug 4, 2021

Conversation

mheyman
Copy link
Contributor

@mheyman mheyman commented Jun 12, 2021

Describe the pull request

  • What does your PR fix?

    New port for json-link (perhaps the fastest JSON deserializer/serializer). Because we don't yet have enough JSON libraries...

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

    all (I hope - only checked windows and linux). No need to update ci.baseline.txt.

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

    Yes.

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

@JonLiu1993 JonLiu1993 self-assigned this Jun 15, 2021
@JonLiu1993 JonLiu1993 changed the title json-link port [json-link] New port Jun 15, 2021
@JonLiu1993 JonLiu1993 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Jun 15, 2021
JonLiu1993
JonLiu1993 requested changes Jun 16, 2021
View reviewed changes
ports/json-link/portfile.cmake Outdated Show resolved Hide resolved
@JonLiu1993
Copy link
Member

@mheyman ,Could you please take a look?
config-x86-windows-out.log

@mheyman
Copy link
Contributor Author

mheyman commented Jun 18, 2021

The failure looks like it occurs in a cmake FetchContent_MakeAvailable() call. This call is identical for linux and windows. The failure occurs after a find_package(Git QUIET) fails to find git. I suspect git is available in the path for linux and osx but not for windows builds.

Darrell Wright's json-link library pulls in headers from two other of his projects. That is what the FetchContent_MakeAvailable() is trying to do.

If FetchContent_MakeAvailable() (with a requirement on git) is unavailable for vcpkg builds, it is possible that I can rework this port into three ports, one for each project. I'd have to patch-out the calls to FetchContent...(). If I do that, I think I should rename the port from json-link to daw-json-link with a dependency on daw-utf-range and daw-header-libraries to make it more obvious they are related (daw-utf-range is also dependent on daw-header-libraries). Much of the header files already land under a daw/ directory (I'm a little dissapointed they don't all land under daw/).

Is that how I should proceed? Make three ports that never call FetchContent_MakeAvailable()? Or, is there an easier solution?

@JackBoosY
Copy link
Contributor

@mheyman For dependent third-party libraries, we have two ways:

  1. Use the port in vcpkg if it exists.
  2. Add vcpkg_from_github or other download methods to pre-download in portfile.cmake, and copy the decompressed folder to source path.

@JonLiu1993
Copy link
Member

@mheyman ，Are you still working on fixing this pr?

@mheyman
Copy link
Contributor Author

mheyman commented Jul 6, 2021 via email

@JackBoosY JackBoosY marked this pull request as draft July 7, 2021 02:03
Neumann-A and others added 18 commits July 10, 2021 14:07
@Neumann-A @mheyman
[vcpkg-tools] update nuget to 5.9.1 (#18447)
9b38654
@NancyLi1013 @mheyman
[vcpkg_download_distfile] Add condition for padding the SHA (#18459)
a6067ec
@traversaro @mheyman
[open6254] Enable uwp support (#18390)
1723d90 
* open62541: Enable uwp support

* Update versions for open62541
@NancyLi1013 @mheyman
[aubio] Add ws2_32 to linkage (#18385)
1af0e45 
* [aubio] Add ws2_32 to linkage

* Update version files
@PhoebeHui @mheyman
[devil] Fix ilut header (#18384)
f1679eb 
* [devil Fix ilut header

* Update baseline
@Neumann-A @mheyman
[libpq] Add Secur32.lib and openssl to libpq cmake wrapper (#18377)
0fd86ab 
* [libpq] add secur32.lib to wrapper

* version stuff

* add openssl fix.

* fix version stuff
@MichaelGoulding @mheyman
[ryml] [c4core] Update RapidYaml (ryml) to newer version (#18230)
53f90f0 
* remove old port version

* fix versions yet again

Co-authored-by: Michael Goulding <[email protected]>
@myd7349 @mheyman
[yyjson] Update to v0.3.0 (#18359)
862d38b 
* [yyjson] Update to 0.3.0

* [yyjson] vcpkg x-add-version yyjson
@Pospelove @mheyman
[raylib] Fix MinGW build (#18362)
999761e 
.
@ianmcook @GPSnoopy
@ras0219-msft @mheyman
[arrow] Update to 4.0.0 (#17975)
356c9fb 
* Update arrow to 4.0.0

* Format

* Try fix thrift

* Update versions/ files

* Do not set ZSTD_ROOT

* Remove double quotes causing Windows problems

* Apply patches

* Remove LIB_DIR_OPTIONS

* Tweak zstd flags

* Update version hash

* Format

* Fail early on x86

* Update hash

* Fail early on arm, arm64

* Update hash

* Add expected failures to to scripts/ci.baseline.txt

* Exclude mallocs from default features

* Update hash

* Set default-features to false for aws-sdk-cpp

Co-authored-by: Robert Schumacher <[email protected]>

* Specify only x64 support in manifest

Co-authored-by: Robert Schumacher <[email protected]>

* Remove unneeded ci.baseline.txt entries

Co-authored-by: Robert Schumacher <[email protected]>

* Remove dataset from default-features

Co-authored-by: Robert Schumacher <[email protected]>

* Update hash

* Remove zstd path args

* Update hash

Co-authored-by: Tanguy Fautre <[email protected]>
Co-authored-by: Robert Schumacher <[email protected]>
@JackBoosY @mheyman
[minizip] Fix usage, fix minizipConfig.cmake (#18472)
3723946 
* [minizip] Fix usage, fix minizipConfig.cmake

* update version record
@dg0yt @mheyman
[windres-rc] Fix wrapper output (#17045)
305cd6a
@Sibras @BillyONeal
@JackBoosY @mheyman
[ffmpeg] Warn/fail when selecting unsupported features. (#17153)
17e4a24 
* [ffmpeg] Warn/fail when selecting unsupported features.

* Update ffmpeg.json

* Bump port-version

* Update ffmpeg.json

* [ffmpeg] Check for gpl/nonfree aswell as all option.

* avisynth now support static linking

* fixup typo

* Update ffmpeg.json

* Revert to fatal error on even when all is selected

* Update ffmpeg.json

* [ffmpeg] Disable openh264 on uwp

* update

* Update versions/f-/ffmpeg.json

* update

* Update ffmpeg.json

* update

* update

* Update

Co-authored-by: Billy Robert ONeal III <[email protected]>
Co-authored-by: Jack·Boos·Yu <[email protected]>
@JonLiu1993 @mheyman
[uwebsockets] update to 19.2.0 (#18525)
38d9bae 
* [uwebsockets] update to <v19.2.0>

* update version
@mcmtroffaes @mheyman
[libass] fix fontconfig dependency in .pc file (#18516)
0962c2e 
* [libass] fix fontconfig dependency in .pc file on x64-linux

* [libass] bump port version

* [libass] x-add-version
@NancyLi1013 @mheyman
[libgpg-error] Remove COPYING.LIB from lib folder (#18492)
dc318e4 
* [libgpg-error] Remove COPYING.LIB from lib folder

* Update version files
@NancyLi1013 @mheyman
[flashlight-cuda] Fix installation (#18489)
13e202e 
* [flashlight-cuda] Fix installation

* Update version files
@gjasny @mheyman
[function2] Update to version 4.2.0 (#18487)
420c0e7
@JackBoosY JackBoosY added requires:author-response depends:upstream-changes Waiting on a change to the upstream project labels Jul 21, 2021
@mheyman mheyman mentioned this pull request Jul 22, 2021
Closed
@beached
Copy link

beached commented Jul 23, 2021

One comment on the testing. It is primarily tested on x64 linux/macos/windows in c++17, in develop C++20 too. It has less primarily tested on 32bit arm/32bit windows but not as a first class. When travis had free CI it was tested on BE ppc and/or S390 I think. It's tested in constant expressions so should work on all the triplets, but I don't know that with certainty

beached added a commit to beached/daw_json_link that referenced this pull request Jul 23, 2021
@beached
Updates to facilitate vcpkg integration
8335ec9 
Updates to facilitate vcpkg integration
microsoft/vcpkg#18411
#247

* Added option to disable FetchContent of dependencies DAW_USE_PACKAGE_MANAGEMENT
* Moved third_part into daw/ subfolder to prevent collisions
* Changed project name to daw-json-link but maintained alias library of
  daw::json_link
@JackBoosY JackBoosY removed the depends:upstream-changes Waiting on a change to the upstream project label Jul 23, 2021
@mheyman
Copy link
Contributor Author

mheyman commented Jul 25, 2021

The latest pull request includes the excellent source updates from beached that removed any need for patch files.

@mheyman
Copy link
Contributor Author

mheyman commented Jul 31, 2021

Header-only comments applied. Sorry for the delay. Life intervened but I did find a second flock of chickens in the neighborhood :-)

JackBoosY
JackBoosY reviewed Aug 2, 2021
View reviewed changes
versions/d-/daw-header-libraries.json Outdated Show resolved Hide resolved
versions/d-/daw-json-link.json Outdated Show resolved Hide resolved
versions/d-/daw-utf-range.json Outdated Show resolved Hide resolved
@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Aug 2, 2021
@dan-shaw dan-shaw merged commit bd5ea16 into microsoft:master Aug 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@beached beached beached left review comments

@JackBoosY JackBoosY JackBoosY approved these changes

@github-actions github-actions[bot] github-actions[bot] approved these changes

@JonLiu1993 JonLiu1993 Awaiting requested review from JonLiu1993

Assignees

@JonLiu1993 JonLiu1993

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.