-
Notifications
You must be signed in to change notification settings - Fork 507
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
Fix MSVS 2019 builds #955
Fix MSVS 2019 builds #955
Conversation
Unfortunately, the clang upgrade that helped fix MSVS 2019 appears to have broken macOS. Not quite ready for review after all. |
If the problem is non-movable, can't you just change from |
I'll definitely try it. Thanks! I've got some other ideas I'd like to discuss to try to simplify the set of changes we need for this. |
deque works perfectly. Much simpler, thanks! I'll make that change. |
https://chromium.googlesource.com/external/gyp/+log/e7079f0e0e14..caa60026e223 In order to use MSVS 2019, gyp must be upgraded. The old version didn't recognize MSVS 2019, but the newer version does. This isn't everything that's needed, but it's a first step. Created with: roll-dep src/packager/tools/gyp Issue #867 (MSVS 2019 support) Issue #336 (progress toward replacing Travis & Appveyor with GitHub Actions, which uses MSVS 2019) b/190743862 (internal; tracking replacement of Travis) Change-Id: I26c433682b1bfd584bf3af0bb4e0bd04646535c1
Because a StreamState object contains a unique_ptr, it is not copyable. A vector of StreamStates, therefore, causes a compile error on resize or push_back, both of which invoke the copy constructor. I don't know why MSVS complains, but clang does not. To fix this, I'm changing vector<StreamState> into deque<StreamState>. At this point static_library builds are working in MSVS 2019. shared_library builds are still not working. Issue #867 (MSVS 2019) Issue #336 (progress toward replacing Travis & Appveyor with GitHub Actions, which uses MSVS 2019) b/190743862 (internal; tracking replacement of Travis) Change-Id: Iaa9d5fc357102d15eac96c29ebeee7c7236e976b
We do not use clang to build on Windows, so we should skip trying to update clang. This is critical to MSVS 2019 support, since the older clang tools we depend on do not recognize that newer environment. Since we don't need clang anyway, skipping the update avoids useless errors about finding a matching clang version for the environment. Issue #867 (MSVS 2019) Issue #336 (progress toward replacing Travis & Appveyor with GitHub Actions, which uses MSVS 2019) b/190743862 (internal; tracking replacement of Travis) Change-Id: I5600ed809b11e68444034a06cc891403e6bfb5cc
The build instructions have been updated with necessary environment variables for newer MSVS versions. Issue #867 (MSVS 2019) Issue #336 (progress toward replacing Travis & Appveyor with GitHub Actions, which uses MSVS 2019) b/190743862 (internal; tracking replacement of Travis) Change-Id: Ic80ba22a750946a508803c52af6b7869964d595b
@TheModMaker helped me simplify things further. Now all the clang-related changes are out, which is good, because the clang changes were breaking macOS. PTAL! |
@@ -111,6 +111,8 @@ hooks = [ | |||
{ | |||
# Pull clang if needed or requested via GYP_DEFINES (GYP_DEFINES="clang=1"). | |||
"name": "clang", | |||
# Skip clang updates on Windows, where we don't use clang. | |||
"condition": "not checkout_win", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you double-check this does run this on Linux/Mac? Just making sure this isn't being ignored and not run anywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just checked, and it still runs the hook on Linux.
Summary of changes:
I don't know why MSVS complains, but clang does not. To fix this, I'm changing vector into deque.
At this point static_library builds are working in MSVS 2019. shared_library builds are still not working.
Closes #867 (MSVS 2019)
Issue #318 (progress toward shared_library support on Windows)
Issue #336 (progress toward replacing Travis & Appveyor with GitHub Actions, which uses MSVS 2019)
b/190743862 (internal; tracking replacement of Travis)