Remove CMake nlohmann module by vhvb1989 · Pull Request #1042 · Azure/azure-sdk-for-cpp

vhvb1989 · 2020-12-01T00:58:13Z

fixes: #1041

Make nlohmann a 3rd party required lib. Do not fetch it with CMake if it is not found

ahsonkhan

Use latest 3.9.1

cmake-modules/add_nlohmann_json.cmake

ghost · 2020-12-01T01:49:17Z

Hello @vhvb1989!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

ahsonkhan · 2020-12-01T18:42:32Z

I am not sure why the CI is failing. For windows, it is failing because of a compilation issue:

azure-sdk-for-cpp/sdk/storage/azure-storage-files-shares/src/share_client.cpp

Line 132 in 9b83bec

    
           if (options.IncludeSnapshots.HasValue() and options.IncludeSnapshots.GetValue())

https://dev.azure.com/azure-sdk/public/_build/results?buildId=636650&view=logs&j=ea3cb55d-7776-5c6b-1bf6-a444b419cb36&t=fa608568-9cc5-5ff2-e699-fab7f8250b1b

share_client.cpp
D:\a\1\s\sdk\storage\azure-storage-files-shares\src\share_client.cpp(132,45): error C2146: syntax error: missing ')' before identifier 'and' [D:\a\1\s\build\sdk\storage\azure-storage-files-shares\azure-storage-files-shares.vcxproj]
D:\a\1\s\sdk\storage\azure-storage-files-shares\src\share_client.cpp(132,45): error C2065: 'and': undeclared identifier [D:\a\1\s\build\sdk\storage\azure-storage-files-shares\azure-storage-files-shares.vcxproj]
D:\a\1\s\sdk\storage\azure-storage-files-shares\src\share_client.cpp(132,49): error C2146: syntax error: missing ';' before identifier 'options' [D:\a\1\s\build\sdk\storage\azure-storage-files-shares\azure-storage-files-shares.vcxproj]
D:\a\1\s\sdk\storage\azure-storage-files-shares\src\share_client.cpp(132,84): error C2059: syntax error: ')' [D:\a\1\s\build\sdk\storage\azure-storage-files-shares\azure-storage-files-shares.vcxproj]
D:\a\1\s\sdk\storage\azure-storage-files-shares\src\share_client.cpp(134,91): error C2059: syntax error: ';' [D:\a\1\s\build\sdk\storage\azure-storage-files-shares\azure-storage-files-shares.vcxproj]
D:\a\1\s\sdk\storage\azure-storage-files-shares\src\share_client.cpp(136,5): error C2059: syntax error: 'return' [D:\a\1\s\build\sdk\storage\azure-storage-files-shares\azure-storage-files-shares.vcxproj]
D:\a\1\s\sdk\storage\azure-storage-files-shares\src\share_client.cpp(140,25): error C2653: 'Models': is not a class or namespace name [D:\a\1\s\build\sdk\storage\azure-storage-files-shares\azure-storage-files-shares.vcxproj]
D:\a\1\s\sdk\storage\azure-storage-files-shares\src\share_client.cpp(140,33): error C2065: 'CreateShareSnapshotResult': undeclared identifier [D:\a\1\s\build\sdk\storage\azure-storage-files-shares\azure-storage-files-shares.vcxproj]
D:\a\1\s\sdk\storage\azure-storage-files-shares\src\share_client.cpp(140,16): error C2923: 'Azure::Core::Response': 'CreateShareSnapshotResult' is not a valid template type argument for parameter 'T' [D:\a\1\s\build\sdk\storage\azure-storage-files-shares\azure-storage-files-shares.vcxproj]
D:\a\1\s\sdk\storage\azure-storage-files-shares\src\share_client.cpp(140,60): error C2653: 'ShareClient': is not a class or namespace name [D:\a\1\s\build\sdk\storage\azure-storage-files-shares\azure-storage-files-shares.vcxproj]
D:\a\1\s\sdk\storage\azure-storage-files-shares\src\share_client.cpp(141,39): error C4430: missing type specifier

And on linux, it is failing because of a unit test failure:
https://dev.azure.com/azure-sdk/public/_build/results?buildId=636650&view=logs&j=fda9a6e2-14f0-5864-d894-5f56622f20fa&t=4d31d3ea-4e03-5255-0249-577ed0f8d090

2020-12-01T01:58:00.1219074Z test 103
2020-12-01T01:58:00.1220028Z         Start 103: azure-core.*/TransportAdapter.connectionPoolTest/*
2020-12-01T01:58:00.1220584Z 
2020-12-01T01:58:00.1222392Z 103: Test command: /home/vsts/work/1/s/build/sdk/core/azure-core/test/ut/azure-core-test "--gtest_filter=*/TransportAdapter.connectionPoolTest/*"
2020-12-01T01:58:00.1223609Z 103: Test timeout computed to be: 10000000
2020-12-01T01:58:00.1270478Z 103: Note: Google Test filter = */TransportAdapter.connectionPoolTest/*
2020-12-01T01:58:00.1271033Z 103: [==========] Running 1 test from 1 test suite.
2020-12-01T01:58:00.1271799Z 103: [----------] Global test environment set-up.
2020-12-01T01:58:00.1272468Z 103: [----------] 1 test from TransportAdapterCurlImpl/TransportAdapter
2020-12-01T01:58:00.1272975Z 103: [ RUN      ] TransportAdapterCurlImpl/TransportAdapter.connectionPoolTest/curlImplementation
2020-12-01T01:58:02.1433207Z 103: /home/vsts/work/1/s/sdk/core/azure-core/test/ut/transport_adapter_curl.cpp:71: Failure
2020-12-01T01:58:02.1434241Z 103: Expected equality of these values:
2020-12-01T01:58:02.1434747Z 103:   Http::CurlConnectionPool::ConnectionsOnPool("httpbin.org")
2020-12-01T01:58:02.1435233Z 103:     Which is: 2
2020-12-01T01:58:02.1435545Z 103:   3
2020-12-01T01:58:02.1435997Z 103: Running Connection Pool Cleaner Test. This test takes more than 3 minutes to complete.
2020-12-01T01:58:02.1437143Z 103: Add compiler option -DRUN_LONG_UNIT_TESTS=OFF when building if you want to skip this test.
2020-12-01T02:01:02.1435930Z 103: First wait time done. Validating state.
2020-12-01T02:01:04.2251029Z 103: [  FAILED  ] TransportAdapterCurlImpl/TransportAdapter.connectionPoolTest/curlImplementation, where GetParam() = 16-byte object <80-72 5A-BD 5C-55 00-00 70-72 5A-BD 5C-55 00-00> (184097 ms)
2020-12-01T02:01:04.2253431Z 103: [----------] 1 test from TransportAdapterCurlImpl/TransportAdapter (184097 ms total)
2020-12-01T02:01:04.2254903Z 103: 
2020-12-01T02:01:04.2256316Z 103: [----------] Global test environment tear-down
2020-12-01T02:01:04.2257693Z 103: [==========] 1 test from 1 test suite ran. (184098 ms total)
2020-12-01T02:01:04.2258887Z 103: [  PASSED  ] 0 tests.
2020-12-01T02:01:04.2259535Z 103: [  FAILED  ] 1 test, listed below:
2020-12-01T02:01:04.2261272Z 103: [  FAILED  ] TransportAdapterCurlImpl/TransportAdapter.connectionPoolTest/curlImplementation, where GetParam() = 16-byte object <80-72 5A-BD 5C-55 00-00 70-72 5A-BD 5C-55 00-00>
2020-12-01T02:01:04.2262293Z 103: 
2020-12-01T02:01:04.2263793Z 103:  1 FAILED TEST
2020-12-01T02:01:04.2283483Z 103/120 Test #103: azure-core.*/TransportAdapter.connectionPoolTest/* .......................***Failed  184.11 sec
...
...
The following tests FAILED:
	103 - azure-core.*/TransportAdapter.connectionPoolTest/* (Failed)

cmake-modules/add_nlohmann_json.cmake

vhvb1989 · 2020-12-01T18:46:04Z

@ahsonkhan , not sure.
However, I will better remove the CMake module and just let this dependency works like libcurl where the end-user needs to take care of installing in before building us.
Had a chat with @antkmsft on our Teams chat and this CMake module approach is not 100% compatible with vcpkg because we can't export the the Json lib target,

…nlohmann-fetchContent

ahsonkhan · 2020-12-01T18:53:50Z

where the end-user needs to take care of installing in before building us.

Hmm...ok. Requiring the user to install a 3rd party json library before they can start to use the SDK in a meaningful way could be a user pain point. Is this a temporary solution?

Is it possible for the user to get the JSON library as part of the SDK installation from vcpkg, automatically?

…ady just like we do with libcurl

vhvb1989 · 2020-12-01T21:28:23Z

where the end-user needs to take care of installing in before building us.

Hmm...ok. Requiring the user to install a 3rd party json library before they can start to use the SDK in a meaningful way could be a user pain point. Is this a temporary solution?

Is it possible for the user to get the JSON library as part of the SDK installation from vcpkg, automatically?

Yes, If an end-user installs, for instance, Azure Storage Blobs SDK, vckpg will first get azure-storage-common, azure-core, libcurl, libxml2, nlohmann-json; compile them and use them tot then build Blobs.

The general experience for vcpkg users is good, I think.

The downside comes to users without vcpkg, (like, directly cloning the repo) where they need to figure it out how to get the dependencies (including now nlohmann json lib).
Or for customer already using a previous beta version from Azure Core. If they update to the next comming version, they will need to first get the json lib or they will be broken for generating the project.

ahsonkhan · 2020-12-01T21:43:04Z

Got it. I think it makes sense to prioritize the vcpkg acquisition experience as that will be our primary release vehicle similar to the other language SDKs. It is OK that there will be a couple extra steps for the "clone from GitHub" scenario.

Or for customer already using a previous beta version from Azure Core. If they update to the next comming version, they will need to first get the json lib or they will be broken for generating the project.

Let's mitigate some of this by updating the docs before the next release and help customers grab the vcpkg easily as part of the upgrade.

sdk/core/azure-core/CMakeLists.txt

ahsonkhan · 2020-12-01T21:46:31Z

sdk/core/azure-core/CMakeLists.txt

+# Storage requires 3.6.0 version for using `contains` feature
+set(NLOHMANN_JSON_MIN_REQUIRED_VERSION 3.6.0)


nit: Consider removing the comment and updating to latest version.

Suggested change

# Storage requires 3.6.0 version for using `contains` feature

set(NLOHMANN_JSON_MIN_REQUIRED_VERSION 3.6.0)

set(NLOHMANN_JSON_MIN_REQUIRED_VERSION 3.9.1)

making it 3.9.1 the MIN will force people to update vcpkg to get latest packages (and not sure if 3.9.1 is already published with vcpkg). I think we have to name the version that comes with the vcpkg version we have tagged right?

I think we have to name the version that comes with the vcpkg version we have tagged right?

Yes! Good call.

https://github.com/microsoft/vcpkg/blob/0bf3923f9fab4001c00f0f429682a0853b5749e0/ports/nlohmann-json/CONTROL#L1-L4

https://github.com/microsoft/vcpkg/releases/tag/2020.11

azure-sdk-for-cpp/eng/vcpkg-commit.txt

Line 1 in 9b83bec

tags/2020.11

guess what! 3.9.1 is the one that comes with tag 2020.11, so yes, let's use that one.

I agree, no need to request 3.9.1 as minimum vbersion if we're fine with 3.6.0.

Unlike libcurl and other dependencies that come with the underlying OS like Ubuntu 16.04 (which we want to use the lowest version possible, for adoption), we should try to use the newest possible library we leverage as part of our implementation, so that we don't carry known bugs.

Requiring a min version of 3.6 doesn't mean it is going to install 3.6. For example, if you run vcpkg install nhollman-json today, it is going to install 3.9.1. You need to have a vcpkg cloned and not updated since Mar 25 2019, in order to get exactly 3.6.0. I don't think it is the job of our repo to request the latest version, when we don't really need it, in order to not get bugs that are not in our code, that we are not affected by, and that we can't enumerate, as overall goodness, especially requesting the latest version - it can be an adoption blocker, if someone is using an earlier version and can't update to the latest for cheap.

What would prevent someone to upgrade the json library to latest that we'd consider it an adoption blocker? Is the adoption blocker concern you mentioned a general one, or specific to this particular library?

How do you reconcile that with security or other bugs that might show up in the older versions of that library? This JSON library IS part of our SDK, since our protocol layers use it as an implementation detail.

I am keeping the 3.6.0 as the MIN.
I consider this a more end-user friendly approach. Specially because we now know that any old application using literals like and will need to be fixed to work with 3.9.0.
So, at least we can have an option for old applications to still go and install 3.6.0 and then use our SDK without any issue.

antkmsft

Please also update readmes where we talk about vcpkg install, to also include nlohmann-json.

ahsonkhan · 2020-12-01T23:07:54Z

FWIW, @vhvb1989 - the master CI is passing, see #1052
The failures are specific to this PR.

…nlohmann-fetchContent

ahsonkhan · 2020-12-03T07:35:25Z

sdk/core/azure-core/CMakeLists.txt


+# Storage requires 3.6.0 version for using `contains` feature
+set(NLOHMANN_JSON_MIN_REQUIRED_VERSION 3.6.0)
+# Try to find the config cmake file. Tipically for when the integration is made at CMake level


Suggested change

# Try to find the config cmake file. Tipically for when the integration is made at CMake level

# Try to find the config cmake file. Typically for when the integration is made at CMake level

version update

accd887

vhvb1989 requested review from ahsonkhan and antkmsft as code owners December 1, 2020 00:58

vhvb1989 self-assigned this Dec 1, 2020

vhvb1989 requested a review from RickWinter as a code owner December 1, 2020 00:58

vhvb1989 changed the title ~~version update~~ version update for CMake nlohmann module Dec 1, 2020

vhvb1989 added Azure.Core Client This issue points to a problem in the data-plane of the library. labels Dec 1, 2020

vhvb1989 added this to the MQ-2020 milestone Dec 1, 2020

antkmsft approved these changes Dec 1, 2020

View reviewed changes

ahsonkhan approved these changes Dec 1, 2020

View reviewed changes

ahsonkhan reviewed Dec 1, 2020

View reviewed changes

cmake-modules/add_nlohmann_json.cmake Outdated Show resolved Hide resolved

cmake-modules/add_nlohmann_json.cmake Outdated Show resolved Hide resolved

vhvb1989 added the auto-merge label Dec 1, 2020

RickWinter approved these changes Dec 1, 2020

View reviewed changes

ahsonkhan reviewed Dec 1, 2020

View reviewed changes

cmake-modules/add_nlohmann_json.cmake Outdated Show resolved Hide resolved

ahsonkhan approved these changes Dec 1, 2020

View reviewed changes

ahsonkhan mentioned this pull request Dec 1, 2020

Update a comment in a cmake file to test if CI is succeeding as expected #1052

Closed

vhvb1989 changed the title ~~version update for CMake nlohmann module~~ Remove CMake nlohmann module Dec 1, 2020

Merge branch 'master' of github.com:Azure/azure-sdk-for-cpp into fix-…

e9b4813

…nlohmann-fetchContent

remove add_nlohmann cmake module. Expect the lib to be installed alre…

e7f11f7

…ady just like we do with libcurl

vhvb1989 force-pushed the fix-nlohmann-fetchContent branch from 2ae219e to e7f11f7 Compare December 1, 2020 21:20

vhvb1989 requested review from a team and danieljurek as code owners December 1, 2020 21:20

vhvb1989 requested review from RickWinter, ahsonkhan and antkmsft December 1, 2020 21:22

vhvb1989 removed the auto-merge label Dec 1, 2020

remove extra find package

ef6e23e

ahsonkhan approved these changes Dec 1, 2020

View reviewed changes

vhvb1989 mentioned this pull request Dec 1, 2020

Update documentation from JS comments Azure/azure-sdk-for-c#1381

Merged

fix for release artifacts

6172761

antkmsft reviewed Dec 1, 2020

View reviewed changes

antkmsft mentioned this pull request Dec 2, 2020

Remove alternative token usage #1053

Merged

ahsonkhan mentioned this pull request Dec 2, 2020

Remove the CMake nlohmann module, use vcpkg to get that dependency, and remove alternative boolean token usage #1054

Closed

vhvb1989 added 2 commits December 3, 2020 01:06

Merge branch 'master' of github.com:Azure/azure-sdk-for-cpp into fix-…

6fb8386

…nlohmann-fetchContent

commets for double find_package

ab5750d

vhvb1989 added the auto-merge label Dec 3, 2020

ghost merged commit c3afb77 into Azure:master Dec 3, 2020

ahsonkhan reviewed Dec 3, 2020

View reviewed changes

This pull request was closed.

		# Storage requires 3.6.0 version for using `contains` feature
		set(NLOHMANN_JSON_MIN_REQUIRED_VERSION 3.6.0)

	# Try to find the config cmake file. Tipically for when the integration is made at CMake level
	# Try to find the config cmake file. Typically for when the integration is made at CMake level

Conversation

vhvb1989 commented Dec 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahsonkhan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ghost commented Dec 1, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

ahsonkhan commented Dec 1, 2020

Uh oh!

Uh oh!

vhvb1989 commented Dec 1, 2020

Uh oh!

ahsonkhan commented Dec 1, 2020

Uh oh!

vhvb1989 commented Dec 1, 2020

Uh oh!

ahsonkhan commented Dec 1, 2020

Uh oh!

Uh oh!

ahsonkhan Dec 1, 2020

Choose a reason for hiding this comment

Uh oh!

vhvb1989 Dec 1, 2020

Choose a reason for hiding this comment

Uh oh!

ahsonkhan Dec 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vhvb1989 Dec 1, 2020

Choose a reason for hiding this comment

Uh oh!

antkmsft Dec 1, 2020

Choose a reason for hiding this comment

Uh oh!

ahsonkhan Dec 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antkmsft Dec 1, 2020

Choose a reason for hiding this comment

Uh oh!

ahsonkhan Dec 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vhvb1989 Dec 3, 2020

Choose a reason for hiding this comment

Uh oh!

antkmsft left a comment

Choose a reason for hiding this comment

Uh oh!

ahsonkhan commented Dec 1, 2020

Uh oh!

ahsonkhan Dec 3, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vhvb1989 commented Dec 1, 2020 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

ahsonkhan Dec 1, 2020 •

edited

Loading

ahsonkhan Dec 1, 2020 •

edited

Loading

ahsonkhan Dec 1, 2020 •

edited

Loading