-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
[abseil] Revert changes about arm build #14109
Conversation
@@ -8,7 +8,6 @@ set(ABSEIL_PATCHES | |||
# This patch is an upstream commit, the related PR: https://github.com/abseil/abseil-cpp/pull/637 | |||
fix-MSVCbuildfail.patch | |||
|
|||
# Remove this patch in next update, see https://github.com/google/cctz/pull/145 |
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.
There may still be problems with this official patch. I have opened an issue (google/cctz#173) upstream to discuss this changes.
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.
Considering they don't support x86 either it seems unlikely to me that they will care about supporting arm
I don't think this change is correct: it "fixes" the build by removing the |
I/we should try to update absl to see if they already have these patches fixed, then I think we should outright remove those associated patches. |
@BillyONeal Trying... |
No, the latest also breaks 32 bit arm build. |
@JackBoosY It breaks arm32 but my argument is that the arm32 build was never correct. It builds successfully only because our old patch is breaking this "alternatename" bit -- not emitting an "alternatename" at all. So the linker doesn't look for the nonexistent symbol so the build succeeds, but the resulting library, which presumably depends on that "alternatename" for users is broken. (And likely nobody noticed because we have relatively few customers who would try to use the resulting library on ARM) I think we should avoid trying to patch in features like this when it's an upstream lack of support that depends on such a fragile patch. |
I opened an upstream issue: abseil/abseil-cpp#829 |
When merging this PR, #11820 should be opened again. |
@BillyONeal Done. |
Co-authored-by: Billy O'Neal <[email protected]>
Co-authored-by: Billy O'Neal <[email protected]>
Thanks :D |
Correct PR #11827, fix build on arm64-uwp.Since the arm32 build issue is an upstream bug, revert the modification about it.
Tested
arm-windows
,arm64-windows
,arm-uwp
,arm64-uwp
,x86-uwp
andx64-uwp
.Fixes #13929.