-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Updated solution to build under Visual Studio 2022 #1720
Updated solution to build under Visual Studio 2022 #1720
Conversation
First, thanks for what was likely a lot of tedious work. I would like to keep the subtree pulls as separate PRs. In order for them to work correctly we have to not squash them. I'm not sure that it was done in the past for Valijson, but even if it wasn't we can make it be so now. I'm assuming that it all still works under 2019 and so pulling in 0.6 should be ok without the rest of your changes. Honestly we should take a look at all of our subtrees and see if we should pull in updates. So you can either wait for us to do that or split Valijson out by itself. If you want to wait, we can prioritize Valijson first. |
Wow! let me echo John's comment of thank you for all of that effort! |
It was done as a separate pr with git subtree and not squashed. |
It works fine under 2019, so I can update the subtree in another PR, and once that gets merged I should be able to merge that change into this branch so it only contains the other changes. I'll mark this as draft until that's done. |
c88a763
to
dbdbdf7
Compare
I rebased the branch on the current master commit (now that Valijson is updated). Much cleaner now :) |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
@jedieaston , I will merge when the build finishes and you approve / comment that you are done. |
Everything looks good to me. Assuming everything passes, I think this is ready to merge. Thanks for your help! |
This PR ended up being way bigger than I thought it would be, but in general, it makes the necessary changes for winget to build using Visual Studio 2022's toolchain (MSVC v143).
Here's what I did:
Updated Valijson to version 0.6. This is the vast majority of the changes, and maybe should be a separate PR (up to y'all). The reason for this is that Valijson made a change to their CMake configuration here to allow it to work properly with Visual Studio. Given that the version currently in the repo is from January, it seemed reasonable to do an update (and since there's a big "External - Do Not Modify" on the vcxitems file, I'm assuming that I shouldn't go cherrypick the change).(edit: this change is actually in Updated Valijson to version 0.6. #1721 now)std::ifstream
has been modified in v143 to be a little more explicit about what its constructor would accept (see<fstream>
: Implement LWG-3430 STL#1968 and the linked C++ spec). As such, several of the usages throughout the project had to be changed to be more explicit (usingTestCommon::TestDataFile.GetPath()
explicitly, and in one instance converting astring_view
to a string). These conversions were happening implicitly before if I understand the spec correctly.AICLI_LOG()
were making it cranky. I think it was because the string literals were being converted and so the compiler couldn't 100% guarantee they would still exist? I'm not 100% sure, but if there's a way to actually fix it then I can do it.If this PR is too big, I can try to move the Valijson changes out (but I couldn't get the project to build without the update, so that would have to go in first). I know review time is limited in this part of the year, so if there's not bandwidth now, I can write down what I did and bring it back sometime later (the project configuration changes are prone to merge conflicts, so I don't know if this could sit in a draft for too too long).
Tested: Ran all tests and builds under Visual Studio 2022 (with workloads configured via .vsconfig file) and Visual Studio 2019 (none of the changes made any difference to v142).
Microsoft Reviewers: Open in CodeFlow