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

[pdal] Prepare for nlohmann-json 3.11.x #26206

Merged
merged 4 commits into from
Aug 8, 2022

Conversation

falbrechtskirchinger
Copy link
Contributor

@falbrechtskirchinger falbrechtskirchinger commented Aug 6, 2022

In nlohmann-json 3.11.0, json_fwd.hpp changed.
Replace PDAL's internal copy of the file as part of the de-vendoring process.

This PR is a requirement for #26124 to move forward.

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

    No change, No

  • 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

In nlohmann-json 3.11.0, json_fwd.hpp changed.
Replace PDAL's internal copy of the file as part of the de-vendoring
process.
@falbrechtskirchinger falbrechtskirchinger changed the title [pdal] Prepare for nlohmann-json 3.11.2 [pdal] Prepare for nlohmann-json 3.11.x Aug 6, 2022
@falbrechtskirchinger falbrechtskirchinger marked this pull request as ready for review August 6, 2022 05:16
@Osyotr
Copy link
Contributor

Osyotr commented Aug 6, 2022

There's no mention of nlohmann-json in pdal's vcpkg.json

@falbrechtskirchinger
Copy link
Contributor Author

I'm confused about that as well, but it clearly references nlohmann/json.hpp. 🤷‍♂️

And nlohmann-json (3.11.x) fails CI as a consequence.

@falbrechtskirchinger
Copy link
Contributor Author

I've no experience with vcpkg and don't use it myself, but I do get the impression there's more wrong with this port.

  • Using "${CURRENT_INSTALLED_DIR}/include/nlohmann/json.hpp" without nlohmann-json being listed as a dependency.
  • Using "${CURRENT_INSTALLED_DIR}/include/nanoflann.hpp" without nanoflann being listed as a dependency.
  • Replacing the bundled single-header json.hpp with the multi-header version.

I could go on but let's start here.

@Osyotr
Copy link
Contributor

Osyotr commented Aug 6, 2022

Using "${CURRENT_INSTALLED_DIR}/include/nlohmann/json.hpp" without nlohmann-json being listed as a dependency.
Using "${CURRENT_INSTALLED_DIR}/include/nanoflann.hpp" without nanoflann being listed as a dependency.

nlohman-json dependency is implicitly satisfied: (pdal -> libgeotiff -> nlohman-json). But it should be explicit.
nanoflann is listed as a dependency of pdal https://github.com/microsoft/vcpkg/blob/39dbc6c45ba9493f1c1b59496d619e0d08f20436/ports/pdal/vcpkg.json#L20

@falbrechtskirchinger
Copy link
Contributor Author

nanoflann is listed as a dependency of pdal

Right. I must have been looking at the wrong file. Thanks for clarifying, I'll add nlohmann-json explicitly.

I'm not a fan of conflating the bundled single-header json.hpp with the installed multi-header json.hpp, but it seems to work.
I believe I understand now why it works as well and that makes my other concerns moot.

@Adela0814 Adela0814 added category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:reviewed Pull Request changes follow basic guidelines labels Aug 8, 2022
@BillyONeal BillyONeal merged commit 82da19b into microsoft:master Aug 8, 2022
@BillyONeal
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants