Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[farmhash] support Windows build #20506

Merged
merged 5 commits into from
Oct 11, 2021
Merged

[farmhash] support Windows build #20506

merged 5 commits into from
Oct 11, 2021

Conversation

luncliff
Copy link
Contributor

@luncliff luncliff commented Oct 4, 2021 •
edited
Loading

What does your PR fix?

Fix makefile build failures in Windows.

Which triplets are supported/not supported? Have you updated the CI baseline?

Removed farmhash annotations in ci.baseline.txt.

  • x64-windows
  • x86-windows
  • x64-uwp
  • x64-windows-static

Does your PR follow the maintainer guide?

Yep. All kind of feedback will be welcomed :D

@luncliff luncliff marked this pull request as draft October 4, 2021 11:09
@luncliff luncliff marked this pull request as ready for review October 5, 2021 00:51
ras0219-msft
ras0219-msft reviewed Oct 5, 2021
View reviewed changes
Copy link
Contributor

@ras0219-msft ras0219-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

ports/farmhash/fix-windows.patch Outdated Show resolved Hide resolved
@JonLiu1993 JonLiu1993 added the category:port-bug The issue is with a library, which is something the port should already support label Oct 8, 2021
@luncliff luncliff mentioned this pull request Oct 9, 2021
Closed
@JonLiu1993
Copy link
Member

@luncliff ,Could you please take a look?

In file included from .././../src/97573ad06d-b8263bbc35.clean/src/farm-test.cc:36:
.././../src/97573ad06d-b8263bbc35.clean/src/farmhash.cc:11846:44: error: missing binary operator before token "("
11846 | #if defined(__has_builtin) && __has_builtin(__builtin_unreachable)
      |                                            ^
make[2]: *** [Makefile:624: farm-test.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [Makefile:473: all-recursive] Error 1
make: *** [Makefile:384: all] Error 2

@luncliff
Copy link
Contributor Author

luncliff commented Oct 9, 2021

@JonLiu1993 Yeah. I'm checking with my laptop now. I will update the part when I find a good answer.

@JonLiu1993 JonLiu1993 added the info:reviewed Pull Request changes follow basic guidelines label Oct 9, 2021
@luncliff luncliff mentioned this pull request Oct 10, 2021
Merged
@BillyONeal BillyONeal merged commit ad6bce2 into microsoft:master Oct 11, 2021
@BillyONeal
Copy link
Member

Thanks for the bugfix!

@luncliff luncliff deleted the port/farmhash branch October 11, 2021 17:29
@luncliff luncliff mentioned this pull request Sep 26, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ras0219-msft ras0219-msft ras0219-msft left review comments

@github-actions github-actions[bot] github-actions[bot] approved these changes

@JonLiu1993 JonLiu1993 JonLiu1993 approved these changes

Assignees

@JonLiu1993 JonLiu1993

Labels
category:port-bug The issue is with a library, which is something the port should already support info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@luncliff @JonLiu1993 @BillyONeal @ras0219-msft