[nlohmann-json] Add option to control implicit conversions behaviour #22409

thomasgt · 2022-01-07T18:07:09Z

This adds an inverse feature disable-implicit-conversions to the nlohmann-json port as described in the associated issue (#22408)

What does your PR fix?

Fixes #22408

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

No changes required

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

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: Local changes detected for nlohmann-json but no changes to version or port version.
-- Version: 3.10.4
-- Old SHA: f948131b4bc6e2e9ae67cb7d40f5e930991fba21
-- New SHA: 4c6c6eeb8001078b424e858b49bd37e37a3990e3
-- Did you remember to update the version or port version?
-- Pass `--overwrite-version` to bypass this check.
***No files were updated.***

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

git add -u && git commit
git checkout 1acb3e1999ee7b9f1e161b4f0937359e01c333e0 -- versions
./vcpkg x-add-version --all

Diff

diff --git a/versions/n-/nlohmann-json.json b/versions/n-/nlohmann-json.json
index bd9d883..29cfd19 100644
--- a/versions/n-/nlohmann-json.json
+++ b/versions/n-/nlohmann-json.json
@@ -1,10 +1,5 @@
 {
   "versions": [
-    {
-      "git-tree": "4c6c6eeb8001078b424e858b49bd37e37a3990e3",
-      "version-semver": "3.10.4",
-      "port-version": 1
-    },
     {
       "git-tree": "f948131b4bc6e2e9ae67cb7d40f5e930991fba21",
       "version-semver": "3.10.4",

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: Local changes detected for nlohmann-json but no changes to version or port version.
-- Version: 3.10.4
-- Old SHA: f948131b4bc6e2e9ae67cb7d40f5e930991fba21
-- New SHA: 4c6c6eeb8001078b424e858b49bd37e37a3990e3
-- Did you remember to update the version or port version?
-- Pass `--overwrite-version` to bypass this check.
***No files were updated.***

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

git add -u && git commit
git checkout 1acb3e1999ee7b9f1e161b4f0937359e01c333e0 -- versions
./vcpkg x-add-version --all

Diff

diff --git a/versions/n-/nlohmann-json.json b/versions/n-/nlohmann-json.json
index bd9d883..29cfd19 100644
--- a/versions/n-/nlohmann-json.json
+++ b/versions/n-/nlohmann-json.json
@@ -1,10 +1,5 @@
 {
   "versions": [
-    {
-      "git-tree": "4c6c6eeb8001078b424e858b49bd37e37a3990e3",
-      "version-semver": "3.10.4",
-      "port-version": 1
-    },
     {
       "git-tree": "f948131b4bc6e2e9ae67cb7d40f5e930991fba21",
       "version-semver": "3.10.4",

dg0yt · 2022-01-07T19:04:12Z

Does your PR follow the maintainer guide?

Yes

https://github.com/microsoft/vcpkg/blob/master/docs/maintainers/maintainer-guide.md#do-not-use-features-to-implement-alternatives

AFAIU this feature is not additive but removes capabilities. You cannot first install a port which requests the feature and then another port which is not ready for it. (Add a first port which requires it here, and suddenly CI sees many failing ports.)

Maybe it is doable with an additive implicit-conversions as a default feature. So you could opt-out for a single port or manifest project if it doesn't depend on the default features.

But there is still some risk of behavioural changes at runtime due to actual conversions changing.

thomasgt · 2022-01-07T19:23:51Z

@dg0yt That makes sense. I was thinking about this from the angle of engaging in beta or preview functionality given that the maintainers of nlohmann json are indicating that this may be the default behaviour in the next major version of the library.

I can appreciate that there is some risk here. Let me know if you think there is a way forward; otherwise, I'll close the PR.

autoantwort · 2022-01-08T02:08:06Z

This would be a use case for boolean options in #22216

dg0yt · 2022-01-08T06:08:44Z

This would be a use case for boolean options in #22216

How would this solve the problem here? Features are hardly different from boolean options. #22216 (comment)

thomasgt · 2022-01-10T13:51:25Z

@autoantwort It's not clear to me that using boolean options would solve the problem that @dg0yt raised. If some project depends on portA and portB, and they both depend on nlohmann-json but each specify different values for implicit-conversions, things may not work. Based on my skimming of the RFC (#22216), using boolean options would result in a failure at package resolution time, whereas using features would potentially fail at compile time, or worse, run time.

If my understanding is correct, vcpkg wants to maintain a self-consistent registry where these types of failures don't happen. I guess that puts the onus on port maintainers to patch packages that depend on existing ports to work with whatever build flags have been selected for the dependencies.

I suppose that means that for this type of change, I shouldn't be introducing a new feature at all. I should just hardcode the flag and then find all the ports that depend on nlohmann-json, test them, and patch them accordingly. This sounds like a lot of work, and it's not clear other port maintainers would even agree to the changes, but then again, these changes may become necessary if the nlohmann-json library maintainers change the default.

strega-nil-ms · 2022-01-10T20:40:05Z

ports/nlohmann-json/portfile.cmake

+vcpkg_check_features(OUT_FEATURE_OPTIONS FEATURE_OPTIONS
+    INVERTED_FEATURES
+        disable-implicit-conversions    JSON_ImplicitConversions
+)
+


Clearly, this doesn't work as a feature; we want features to be additive, and this is a breaking change.

I think the best thing to do here is to make it a triplet setting; something like:

if(NOT DEFINED nlohmann-json_IMPLICIT_CONVERSIONS) set(nlohmann-json_IMPLICIT_CONVERSIONS ON) endif()

and then pass it to vcpkg_cmake_configure as:

OPTIONS ... "-DJSON_ImplicitConversions=${nlohmann-json_IMPLICIT_CONVERSIONS}"

Ok, that sounds like a better solution - thanks! Is there anywhere I should document that this triplet setting exists in case anyone else wants to use it?

@strega-nil for opinion

There's docs/users/triplets.md but I'm not in favor of adding port-specific documentation to it. As far as I'm aware, this would be the first variable of its kind and it makes sense for it to be documented inside the port (maybe adding a readme.md file?).

autoantwort · 2022-01-23T20:43:55Z

@autoantwort It's not clear to me that using boolean options would solve the problem that @dg0yt raised. If some project depends on portA and portB, and they both depend on nlohmann-json but each specify different values for implicit-conversions, things may not work. Based on my skimming of the RFC (#22216), using boolean options would result in a failure at package resolution time, whereas using features would potentially fail at compile time, or worse, run time.

Exactly, instead of failing at compile time or run time it would fail at resolution time. Imho that is a lot better.

If you now use a triplet variable for that, you have the same negative effects as when you use a feature. And you use some magic triplet var and not a option specified in the vcpkg.json that you can simply set as a user without writing a custom triplet for every target triplet.

thomasgt · 2022-01-24T14:24:12Z

@autoantwort I agree that failing at package resolution time is preferred over failing at compile time or runtime. I also agree that my initial idea of implementing this as a feature won't work.

All that being said, I could imagine someone being frustrated because they are trying to depend on two libraries, libA and libB, that both depend on nlohmann-json but each specify a different value for the implicit conversion option. There's no way to reconcile the situation; the person trying to use vcpkg will be stuck.

My interpretation of vcpkg's stance on this situation is that they'd prefer the port maintainers of libA and libB to deal with this problem via patches. That way vcpkg can provide a cohesive environment where port consumers can never get stuck with an unsatisfiable dependency tree. @strega-nil-ms is my interpretation correct?

I don't mind using a triplet variable for this. It's on me to verify things work if I use it. My only question is where I'd document that the triplet variable exists so others can use it if they want.

github-actions

You have modified or added at least one vcpkg.json where a "license" field is missing.
If you feel able to do so, please consider adding a "license" field to the following files:

ports/nlohmann-json/vcpkg.json

Valid values for the license field are listed at https://spdx.org/licenses/

github-actions

You have modified or added at least one vcpkg.json where a "license" field is missing.
If you feel able to do so, please consider adding a "license" field to the following files:

ports/nlohmann-json/vcpkg.json

Valid values for the license field are listed at https://spdx.org/licenses/

ras0219-msft · 2022-03-02T00:30:14Z

It's on me to verify things work if I use it. My only question is where I'd document that the triplet variable exists so others can use it if they want.

This triplet variable can be documented as part of a usage file, which will be displayed to the user instead of our heuristically generated usage information. Without a share/nlohmann-json/usage file, we currently print:

PS C:\src\vcpkg> ./vcpkg install nlohmann-json:x64-windows
/ * skipped some output here */

The package nlohmann-json provides CMake targets:

    find_package(nlohmann_json CONFIG REQUIRED)
    target_link_libraries(main PRIVATE nlohmann_json nlohmann_json::nlohmann_json)

Typically, ports that override usage will have a replacement usage text file in the port directory and copy it into place:

portfile.cmake

file(COPY "${CMAKE_CURRENT_LIST_DIR}/usage" DESTINATION "${CURRENT_PACKAGES_DIR}/share/${port}")

usage

The package nlohmann-json provides CMake targets:

    find_package(nlohmann_json CONFIG REQUIRED)
    target_link_libraries(main PRIVATE nlohmann_json::nlohmann_json)

The package nlohmann-json can be configured to not provide implicit conversions via a custom triplet file:

    set(nlohmann_json_JSON_ImplicitConversions OFF)

github-actions

You have modified or added at least one vcpkg.json where a "license" field is missing.

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

ports/nlohmann-json/vcpkg.json

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

cenit · 2022-03-24T15:55:58Z

usage tells you one variable, then the portfile internally checks another one...

thomasgt · 2022-03-24T18:19:21Z

@cenit Whoops, that was a copy-paste error on my part. Will push a fix.

* master: (103 commits) [lmdb] don't use msvc parameters with non-msvc compiler (microsoft#23653) Fix <version> of Python in vcpkgTools.xml (microsoft#23751) [sciter] escape quotes (microsoft#23752) [fuzzylite] Fix Linux build (microsoft#23658) [cpp-redis] Fix ‘sleep_for’ is not a member of ‘std::this_thread’ (microsoft#23762) [nlohmann-json] Fix usage text (microsoft#23749) [fontconfig] Do not create symlinks (microsoft#23735) (microsoft#23736) [winsparkle] Fix header file and debug path (microsoft#23739) [arb] Support dynamic build (microsoft#23743) [aws-sdk-cpp] update to 1.9.220 (microsoft#23729) Fix the VS2022 'unstable' queues. (microsoft#23742) [xmlsec] Bump to 1.2.33 (microsoft#23733) [unicorn] update to latest version v1.0.3 (microsoft#23745) [libpq] Update version to 14.1 2 (microsoft#22516) [libnoise] Export CMake files (microsoft#23682) [vcpk-ci] Trigger some test ports from vcpkg.cmake changes (microsoft#23430) [nlohmann-json] Add option to control implicit conversions behaviour (microsoft#22409) [libjuice] Update to 0.9.8 (microsoft#23153) [libgeotiff] Update to 1.7.1 (microsoft#23446) [minizip-ng] Updated minizip version and fixed windows build for previous version (microsoft#23684) ...

github-actions bot requested changes Jan 7, 2022

View reviewed changes

thomasgt force-pushed the master branch from 564610a to 5ef6c08 Compare January 7, 2022 18:09

github-actions bot requested changes Jan 7, 2022

View reviewed changes

thomasgt force-pushed the master branch from 5ef6c08 to 028f41a Compare January 7, 2022 18:31

github-actions bot approved these changes Jan 7, 2022

View reviewed changes

JonLiu1993 assigned LilyWangLL Jan 10, 2022

JonLiu1993 added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Jan 10, 2022

LilyWangLL approved these changes Jan 10, 2022

View reviewed changes

LilyWangLL added the info:reviewed Pull Request changes follow basic guidelines label Jan 10, 2022

strega-nil-ms added requires:discussion and removed info:reviewed Pull Request changes follow basic guidelines requires:discussion labels Jan 10, 2022

strega-nil-ms suggested changes Jan 10, 2022

View reviewed changes

strega-nil-ms added the requires:author-response label Jan 10, 2022

thomasgt force-pushed the master branch from 028f41a to 019f054 Compare February 8, 2022 19:53

github-actions bot approved these changes Feb 8, 2022

View reviewed changes

thomasgt requested a review from strega-nil-ms February 8, 2022 20:01

github-actions bot approved these changes Feb 8, 2022

View reviewed changes

JackBoosY removed the requires:author-response label Feb 14, 2022

LilyWangLL changed the title ~~[nlohmann-json] Add port feature to control implicit conversions behaviour~~ [nlohmann-json] Add option to control implicit conversions behaviour Feb 14, 2022

LilyWangLL added the info:reviewed Pull Request changes follow basic guidelines label Feb 21, 2022

ras0219-msft removed the info:reviewed Pull Request changes follow basic guidelines label Mar 2, 2022

ras0219-msft added the requires:author-response label Mar 2, 2022

thomasgt added 3 commits March 14, 2022 10:16

[nlohmann-json] Add "implicit-conversions" triplet variable

dcae834

[nlohmann-json] Customize usage to document triplet var

068cd7b

[nlohmann-json] Update version files

8f0444a

thomasgt force-pushed the master branch from bdd1a3e to 8f0444a Compare March 14, 2022 13:56

github-actions bot approved these changes Mar 14, 2022

View reviewed changes

thomasgt requested review from vicroms and LilyWangLL March 23, 2022 12:49

vicroms approved these changes Mar 23, 2022

View reviewed changes

vicroms added the info:reviewed Pull Request changes follow basic guidelines label Mar 23, 2022

dan-shaw merged commit 6ad0750 into microsoft:master Mar 23, 2022

thomasgt mentioned this pull request Mar 24, 2022

[nlohmann-json] Fix usage text #23749

Merged

Osyotr mentioned this pull request Apr 8, 2022

[osg] Add GL3 feature to OSG #24049

Closed

JonLiu1993 mentioned this pull request Apr 20, 2022

[libxslt] add optional feature support #24240

Merged

vicroms mentioned this pull request Oct 20, 2022

[RFC] Options 🎉 #22216

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[nlohmann-json] Add option to control implicit conversions behaviour #22409

[nlohmann-json] Add option to control implicit conversions behaviour #22409

thomasgt commented Jan 7, 2022 •

edited

Loading

github-actions bot left a comment

github-actions bot left a comment

dg0yt commented Jan 7, 2022

Does your PR follow the maintainer guide?

thomasgt commented Jan 7, 2022

autoantwort commented Jan 8, 2022

dg0yt commented Jan 8, 2022

thomasgt commented Jan 10, 2022

strega-nil-ms Jan 10, 2022

thomasgt Jan 14, 2022

vicroms Feb 24, 2022

autoantwort commented Jan 23, 2022

thomasgt commented Jan 24, 2022

github-actions bot left a comment

github-actions bot left a comment

ras0219-msft commented Mar 2, 2022

github-actions bot left a comment

cenit commented Mar 24, 2022

thomasgt commented Mar 24, 2022

[nlohmann-json] Add option to control implicit conversions behaviour #22409

[nlohmann-json] Add option to control implicit conversions behaviour #22409

Conversation

thomasgt commented Jan 7, 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

github-actions bot left a comment

Choose a reason for hiding this comment

dg0yt commented Jan 7, 2022

Does your PR follow the maintainer guide?

thomasgt commented Jan 7, 2022

autoantwort commented Jan 8, 2022

dg0yt commented Jan 8, 2022

thomasgt commented Jan 10, 2022

strega-nil-ms Jan 10, 2022

Choose a reason for hiding this comment

thomasgt Jan 14, 2022

Choose a reason for hiding this comment

vicroms Feb 24, 2022

Choose a reason for hiding this comment

autoantwort commented Jan 23, 2022

thomasgt commented Jan 24, 2022

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

ras0219-msft commented Mar 2, 2022

github-actions bot left a comment

Choose a reason for hiding this comment

cenit commented Mar 24, 2022

thomasgt commented Mar 24, 2022

thomasgt commented Jan 7, 2022 •

edited

Loading

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