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.

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

dev-cpp/folly: fix build on ppc64 #29393

Closed
wants to merge 1 commit into from
Closed

Conversation

darkbasic
Copy link
Contributor

@darkbasic darkbasic commented Feb 2, 2023

Bug: https://bugs.gentoo.org/892942

Not setting CMAKE_LIBRARY_ARCHITECTURE defaults it to x86_64 and force enables SSE4_2 breaking all platforms but amd64. Setting anything but an empty string (or x86_64) will do the trick. This PR conditionally sets CMAKE_LIBRARY_ARCHITECTURE on ppc64, but we should probably set it on all platforms. Unfortunately I'm not familiar with CMAKE_LIBRARY_ARCHITECTURE so I'm not sure if x86_64 is arbitrary or standard naming convention (might as well use something like powerpc64le-unknown-linux-gnu AFAIK). It looks like amd64 needs x86_64 specifically but other archs are not hardcoded yet. Also, shouldn't SSE4_2 be behind a use flag even on amd64? Not sure if compiling it with SSE4_2 actually requires it at runtime.

Upstream issue: facebook/folly#1851

@gentoo-bot
Copy link

Pull Request assignment

Submitter: @darkbasic
Areas affected: ebuilds
Packages affected: dev-cpp/folly

dev-cpp/folly: @thesamesam

Linked bugs

Bugs linked: 892942


In order to force reassignment and/or bug reference scan, please append [please reassign] to the pull request title.

Docs: Code of ConductCopyright policy (expl.) ● DevmanualGitHub PRsProxy-maint guide

@gentoo-bot gentoo-bot added assigned PR successfully assigned to the package maintainer(s). bug linked Bug/Closes found in footer, and cross-linked with the PR. labels Feb 2, 2023
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2023-02-02 14:03 UTC
Newest commit scanned: fb5f926
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/92da0b887a/output.html

@darkbasic
Copy link
Contributor Author

Build log with the patch: build.log

@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2023-02-03 15:03 UTC
Newest commit scanned: 742398e
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/1e42d336bc/output.html

@darkbasic darkbasic marked this pull request as ready for review February 15, 2023 09:18
@darkbasic darkbasic changed the title [DRAFT] dev-cpp/folly: fix build on ppc64 dev-cpp/folly: fix build on ppc64 Feb 15, 2023
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2023-02-15 09:28 UTC
Newest commit scanned: 401e1be
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/d78c0bf643/output.html

@thesamesam
Copy link
Member

Thanks, could you:

  1. rebase;
  2. add a comment above the line with a ref to the upstream bug / our discussion here / the Gentoo bug, so we don't try clean it up in future etc?
  3. write ${ARCH} style, not $ARCH

cheers!

Bug: https://bugs.gentoo.org/892942
Signed-off-by: Niccolò Belli <niccolo.belli@linuxsystems.it>
@darkbasic
Copy link
Contributor Author

Done. I personally prefer to git blame (and I've already referenced the Gentoo bug in the commit) but I guess that not everybody likes to use git.

@thesamesam
Copy link
Member

thesamesam commented Feb 17, 2023

git blame doesn't work well for ebuilds if there's no live ebuild, as the filename changes, and it doesn't always do a good job of handling renames. Plus it's about ease of lookup. It's a lot easier for me to check on it and remember why it's there if it's sitting in front of me. Thanks!

@darkbasic
Copy link
Contributor Author

Oh you're completely right, I didn't think about the filename changes.

@barracuda156
Copy link

@darkbasic This is somewhat misleading, since ppc64 is Big-endian architecture, and just fixing a wrong assumption in CMakeLists does not in itself fix folly for ppc64. There are several places in the code where static asserts gonna force the build fail.

@thesamesam
Copy link
Member

No, ppc64 can be both big-endian and little-endian. I believe @darkbasic uses ppc64 on little-endian.

@barracuda156
Copy link

PowerPC cpus are bi-endian, but ppc64 is a big-endian arch. There is ppc64le for LE.

@thesamesam
Copy link
Member

In Gentoo, ppc64 is either. We're discussing this in the context of Gentoo. We have profiles for handling endianness for ppc64.

@barracuda156
Copy link

Got it, thanks for clarification.

Do you know if someone was able to make it work on big-endian ppc64 on Gentoo? I ask since the current master branch of folly will not allow to build the thing without patching out at least this: facebook/folly#1951
Building with tests will introduce more problems (not sure how many yet, I am trying to get through it right now).

@darkbasic
Copy link
Contributor Author

Do you know if someone was able to make it work on big-endian ppc64 on Gentoo?

Not sure, my system is little endian with 4K page size so I get the least amount of issues.

@barracuda156
Copy link

Do you know if someone was able to make it work on big-endian ppc64 on Gentoo?

Not sure, my system is little endian with 4K page size so I get the least amount of issues.

If you or anyone will be able to test (and possibly address) likely issues on Big endian (maybe via Qemu, it is supported on Linux), it would be greatly appreciated.

P. S. For the context: I maintain folly in Macports, and while I have fixed the basic build for all supported OS back to my PowerPC systems, at present it omits Base64SWAR, which, as it looks, is now required for new fizz, and also build with tests is pretty broken.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assigned PR successfully assigned to the package maintainer(s). bug linked Bug/Closes found in footer, and cross-linked with the PR.
Projects
None yet
5 participants