Skip to content

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

Closed
ahsonkhan wants to merge 6 commits intoAzure:masterfrom
ahsonkhan:RemoveNlohmannAndBoolFix
Closed

Remove the CMake nlohmann module, use vcpkg to get that dependency, and remove alternative boolean token usage#1054
ahsonkhan wants to merge 6 commits intoAzure:masterfrom
ahsonkhan:RemoveNlohmannAndBoolFix

Conversation

@ahsonkhan
Copy link
Contributor

@ahsonkhan ahsonkhan commented Dec 2, 2020

Merging #1042 and #1053 in an effort to isolate the root cause of the recent CI failure.

@ahsonkhan ahsonkhan self-assigned this Dec 2, 2020
@ahsonkhan ahsonkhan added this to the [2020] December milestone Dec 2, 2020
@ahsonkhan ahsonkhan added the EngSys This issue is impacting the engineering system. label Dec 2, 2020
@ahsonkhan
Copy link
Contributor Author

FYI @vhvb1989, @antkmsft

@vhvb1989
Copy link
Member

vhvb1989 commented Dec 2, 2020

FYI @vhvb1989, @antkmsft

I see green for Windows legs, so, it looks like this was it. hahaha,, MSVC being MSVC! LoL.
Funny how we will try to be non-breaking but MSVC could break us at any time (this is not the first time we get something like this).
I wonder if there is a way how we can be involved with the next MSCV version before it is releases and have maybe a CI leg or nightly leg for it so we can act before and not after it happen

@ahsonkhan
Copy link
Contributor Author

ahsonkhan commented Dec 2, 2020

The question remains why did using vcpkg and updating the JSON library uncover this MSVC issue? What is unique about version 3.9.0/3.9.1 that is preventing our SDK from using and? The 3rd party library should not have such a side effect on our SDK build for an unrelated source file.
Maybe this experiment will help: #1055

@vhvb1989
Copy link
Member

vhvb1989 commented Dec 2, 2020

The question remains why did using vcpkg and updating the JSON library uncover this MSVC issue? What is unique about version 3.9.0/3.9.1 that is preventing our SDK from using and? The 3rd party library should not have such a side effect on our SDK build for an unrelated source file.
Maybe this experiment will help: #1055

The question remains why did using vcpkg and updating the JSON library uncover this MSVC issue? What is unique about version 3.9.0/3.9.1 that is preventing our SDK from using and? The 3rd party library should not have such a side effect on our SDK build for an unrelated source file.
Maybe this experiment will help: #1055

For how it looks, the json library added some CMake code to make the build vulnerable to this. Something like changing a compile flag. I will try to see what changed.

@antkmsft
Copy link
Member

antkmsft commented Dec 2, 2020

@ahsonkhan , @vhvb1989 I don't think it's due to vcpkg dependency, it is timing coincidence. I had it repro on my machine as well. Some CI/CD VMs picked up the change, some didn't, this is why we saw it occasionally.

@vhvb1989
Copy link
Member

vhvb1989 commented Dec 2, 2020

@ahsonkhan , @vhvb1989 I don't think it's due to vcpkg dependency, it is timing coincidence. I had it repro on my machine as well. Some CI/CD VMs picked up the change, some didn't, this is why we saw it occasionally.

I checked the MSVC version from the failed and passed CI gates and it is the same.
That´s what I don´t fully understand.
Also, when I started my Windows Machine (it was shutdown since long ago) I din´t update visual studio and still saw the issue.
So, I am more into the idea that the json lib on its latest version is setting some build config that exposes this issue. That´s why if we currently keep adding more dummy changes (without updating the json lib), we would still get green builds.

@ahsonkhan
Copy link
Contributor Author

ahsonkhan commented Dec 2, 2020

I don't think it's due to vcpkg dependency, it is timing coincidence.

It reproduces quite consistently in CI as soon as you upgrade the dependency to 3.9.1 which makes me think it doesn't have anything to do with a timing issue.
https://dev.azure.com/azure-sdk/public/_build/results?buildId=638486&view=logs&j=ea3cb55d-7776-5c6b-1bf6-a444b419cb36&t=fa608568-9cc5-5ff2-e699-fab7f8250b1b

So, I am more into the idea that the json lib on its latest version is setting some build config that exposes this issue. That´s why if we currently keep adding more dummy changes (without updating the json lib), we would still get green builds.

Yep, that's what it looks like to me.
See this PR failing when all we did was upgrade from 3.8.0 to 3.9.1: #1055

Now, why is the version bump causing CI failures because of the use of and boolean elsewhere. @vhvb1989, can you file an issue to https://github.com/nlohmann/json/issues and see if the library author (@nlohmann) can provide some insights?

@ahsonkhan
Copy link
Contributor Author

Root cause found to be nlohmann/json#2089 and nlohmann/json@8d29523

Closing this.

@ahsonkhan ahsonkhan closed this Dec 3, 2020
@ahsonkhan ahsonkhan deleted the RemoveNlohmannAndBoolFix branch December 3, 2020 00:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

EngSys This issue is impacting the engineering system.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants