Bug 213637
| Summary: | net/freerdp: Fix build on ARMv6, Clean up w/ _CMAKE_BOOL | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | Ports & Packages | Reporter: | Kyle Evans <kevans> | ||||||||||||||||
| Component: | Individual Port(s) | Assignee: | Ben Woods <woodsb02> | ||||||||||||||||
| Status: | Closed FIXED | ||||||||||||||||||
| Severity: | Affects Some People | CC: | jbeich, kevans, mikael, woodsb02 | ||||||||||||||||
| Priority: | --- | Keywords: | easy, needs-qa, patch | ||||||||||||||||
| Version: | Latest | Flags: | koobs:
maintainer-feedback+
koobs: merge-quarterly? |
||||||||||||||||
| Hardware: | arm | ||||||||||||||||||
| OS: | Any | ||||||||||||||||||
| Bug Depends on: | 212004 | ||||||||||||||||||
| Bug Blocks: | |||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
(In reply to Kyle Evans from comment #0) Can we add BROKEN for OSVERSION's < of some values so that an appropriate message can be provided to the user to update? armv6hf is dead, you can remove it. @koobs: Should the BROKEN state a minimum revision to upgrade past, or what's the best practice here? I can generally narrow it down to $OSVERSION >= 1100504 and >= 1200008 (unless/til the libm changes works it way down to stable/10 -- I'm guessing stable/9 will be out of the question at this point). Right now I'm setting BROKEN= net/freerdp will not build on this version of FreeBSD. Please update to a more recent version -- but this doesn't feel quite right. @mikael: Noted and revised -- will be gone in the next iteration of this patch. (In reply to Kyle Evans from comment #3) Appropriate messaging in this case probably mentions the revision that originally fixed it. Something like: BROKEN=Fails to build on ARMv6 (libm bug). See: r<revision> [and #<Issue-ID>]. Update to a later (fixed) version. Created attachment 176004 [details]
Revised svn(1) diff of net/freerdp
Revised version to address armv6hf not being a (living) thing anymore and koob@'s request for a formal BROKEN w/ information.
The conditionals for BROKEN= are kind of convoluted, but I tried to break it up into OPSYS/ARCH checks and then all of the different OSVERSION ranges. This affects (in the order written in the Makefile):
1.) Anything before 1100500 (includes 9.x, 10.x, release/11.0.[01])
2.) stable/11, up to 1100504
3.) HEAD, up to 1200008
If it ends up in stable/10, I'll re-adjust OSVERSION to reflect this.
I left the #1 case separate from the #2 case to make it obvious that the pre-110500 was intentional and not an oversight, despite being redundant.
@Kyle Nice work, thank you for the quick update. I'll take care of committing this tonight for you. I presume it still passes your local QA (In reply to Kubilay Kocak from comment #6) Hi, Whoops, sorry, forgot to mention that. =) Indeed- it does. portlint isn't necessarily happy about the patch naming, but I don't know that it's worth touching the svn(1) mv's when this port will effectively receive no future changes. I have a net/freerdp2 port in my pocket that I've neglected to create a PR for that will generally hold any future development. I'm not sure it's worth putting all these checks for armv6, we should just check for version < 11.0-RELEASE or 1200000 < version < 1200008 and encourage people to upgrade their arm system. Have you tried to build it with neon enabled? neon is a armv7 feature and sometime you need to put the correct cflags (-march=armv7a -mfpu=neon) (In reply to mikael.urankar from comment #8) Hi, ARMv6 on 11.0-RELEASE also has the issue mentioned, and it wasn't fixed until a bit into stable/11 (so, after 11.0-RELEASE), so it's a little more complicated than < 11.0-RELEASE or 1200000 < OSVERSION < 1200008, I'm afraid, since most (all -RELEASE, at the very least) versions of FreeBSD *can not* currently compile net/freerdp for ARMv6. It builds with NEON by default, hence the reason for adding the NEON option and explicitly disabling it otherwise. I have no doubt that building with NEON works, but it doesn't work when the system you're building on is actually ARMv6 -- this is the case with at least the build cluster and my local stuff. Also unfortunately, we can't easily detect NEON support or that we're building on an ARMv7 system because one $ARCH covers both ARMv6 and ARMv7. Therefore, it's safer to take the multimedia/ffmpeg route and just provide it as an option so that those who know what they're doing and want to enable it can do so, but it doesn't break building for people that want it to *just work* as it is. Thanks for the clarification on OSVERSION, all these numbers confused me. The problem with neon is that you provide a broken option. It can be fixed with the correct cflags. ffmpeg does it the right way by selecting the correct cpu if you enable neon (NEON_CONFIGURE_ENABLE= neon, NEON_CONFIGURE_ON= --cpu=armv7-a) All you need to do is to add CFLAGS+= -march=armv7-a when NEON option is on Another thing, we switched to hardfloat abi some time ago, you need to set ARM_FP_ABI=hard (cf cmake/ConfigOptions.cmake), with or without neon. finally you can 'hide' the sse option on armv6: OPTIONS_EXCLUDE_armv6=SSE Created attachment 176027 [details]
Revised^2 svn(1) diff of net/freerdp
(In reply to mikael.urankar from comment #10) @OSVERSION: Yeah -- this will become simpler over time, but unfortunately it is a mess right now. @NEON: Ahh, okay, for some reason I thought their CMake foo actually took care of any -march= stuff it needed, but reviewing it again it only adds -mfpu. Updated patch should fix this and set the ARM_FP_ABI option. Thanks! =) Created attachment 176028 [details]
Revised^2 svn(1) diff of net/freerdp
Created attachment 176029 [details]
Revised svn(1) diff of net/freerdp
...re-read the last post.
Created attachment 176030 [details]
(Whitespace fix) svn(1) diff of devel/freerdp
Apologies, this should be the last one -- made some unnecessary whitespace changes, and then the machine I'm on right now messed up the line endings.
Created attachment 176032 [details]
svn(1) diff of net/freerdp
Alright, for real this time. =) After fixing a capitalization error, confirmed with mikael that it builds properly for him with and without NEON. Confirmed locally that it still builds on non-ARM, so all is good now.
Comment on attachment 176032 [details] svn(1) diff of net/freerdp > +CMAKE_ARGS+= ... -DARM_FP_ABI=hard Before base r300119 FreeBSD armv6 was -mfloat-abi=softfp by default. I guess, ports/ don't support armv6 on 10.x. > +OPTIONS_DEFINE_armv6= NEON > +OPTIONS_DEFINE_aarch64= NEON Why do you disable NEON by default for aarch64? It's a standard extension on ARMv8. $ clang -target aarch64--freebsd -dM -E -</dev/null |& fgrep NEON #define __ARM_NEON 1 #define __ARM_NEON_FP 0xE > +NEON_CFLAGS= -march=armv7-a This would break on aarch64. $ clang -target aarch64--freebsd -march=armv7-a -dM -E -</dev/null clang: error: the clang compiler does not support '-march=armv7-a' (In reply to Jan Beich (mail not working) from comment #17) Hi, =) > Before base r300119 FreeBSD armv6 was -mfloat-abi=softfp by default. I guess, ports/ don't support armv6 on 10.x. To be clear, would you recommend doing this in another way? It seems like I might do well to replace this with an ARM_FP_ABI make(1) variable with a default?= hard -- but this seems like an approach that may not necessarily be consistent with others? Thoughts? > Why do you disable NEON by default for aarch64? It's a standard extension on ARMv8. Honestly, because I chose to follow the approach of multimedia/ffmpeg, which also has it off by default. This makes sense, though, given that all aarch64 CPUs -will- have NEON. >> +NEON_CFLAGS= -march=armv7-a > This would break on aarch64. Ah, a fair point indeed. =) Would it be safe/sensible to wrap this in an .if ${ARCH} != "aarch64", or is there a better way that I'm unaware of? Apologies for the questions, just want to make sure that I do things the right way. =) (In reply to Kyle Evans from comment #18) > would you recommend doing this in another way? Drop -mfloat-abi, /usr/share/mk/bsd.cpu.mk already adds it to CFLAGS if necessary. On aarch64 whether -mfpu and -mfloat-abi are ignored (e.g., Clang) or rejected (e.g., GCC) depends on the compiler. Also, -mfpu=neon is nop with Clang when building for -march=armv7-a but not with GCC. Try the following NEON_CFLAGS= -march=armv7-a -mfpu=neon [...] post-patch: ${REINPLACE_CMD} -e '/-mfloat-abi/d' \ ${WRKSRC}/libfreerdp/CMakeLists.txt unless you want to patch the source then forward upstream. >> Why do you disable NEON by default for aarch64? It's a standard extension on ARMv8. > because I chose to follow the approach of multimedia/ffmpeg ffmpeg lacks OPTIONS_DEFINE_aarch64 which let's configure auto-detect NEON and VFP. OTOH, freerdp tries to enable NEON for any "arm", except aarch64 isn't one due to lack of the substring. ;) Try the following instead CMAKE_ARGS= ${CMAKE_ARGS_${ARCH}} CMAKE_ARGS_aarch64= -DWITH_NEON=on CFLAGS_aarch64= -D__ARM_NEON__=__ARM_NEON # clang unless you want to patch the source then forward upstream. > .if ${ARCH} != "aarch64" Do you see such a conditional in multimedia/ffmpeg ? Let's not complicate the code. Also, a user may optimize for newer model than armv7-a via CPUTYPE in /etc/make.conf, so don't append -march if already present: .if ! ${CFLAGS:M-march*} NEON_CFLAGS= -march=armv7-a .endif bsd.cpu.mk is processed before Makefile, so such a conditional doesn't require an .include line before. (In reply to Jan Beich (mail not working) from comment #19) Excellent- I'll patch the source to drop the -mfloat-abi -- this is actually a non-issue in newer versions of FreeRDP, because upstream already chose to drop this since it caused more hassle than it alleviated. I'll leave -mfpu as it is, given that your description makes it sound harmless when used as it is. Given the aarch64 situation, it makes sense to: 1.) Not provide the option for aarch64, and 2.) Use the mentioned bits to enable WITH_NEON and pass in the mentioned -D__ARM_NEON__ #define. Yeah? A commit references this bug: Author: woodsb02 Date: Sun Dec 11 06:11:08 UTC 2016 New revision: 428332 URL: https://svnweb.freebsd.org/changeset/ports/428332 Log: net/freerdp1: Implement port improvements made to net/freerdp in r428330 - Remove DIRECTFB option, as it no longer compiles, and gets little upstream maintenance - Use NEON on aarch64, and optionally on armv6 - Mark as broken on armv6 on FreeBSD 11.0-RELEASE and early versions of 12.0-CURRENT - Re-generate patches (pet portlint) PR: 212004 PR: 213637 Submitted by: Kyle Evans (maintainer) Reviewed by: Mikael Urankar <mikael.urankar@gmail.com> Reviewed by: koobs Reviewed by: jbeich Approved by: adamw (mentor, implicit) Changes: head/net/freerdp1/Makefile head/net/freerdp1/distinfo head/net/freerdp1/files/patch-CMakeLists.txt head/net/freerdp1/files/patch-cmake-FindOpenSSL.cmake head/net/freerdp1/files/patch-cmake_FindGStreamer_1_0.cmake head/net/freerdp1/files/patch-cmake_FindGStreamer__1__0.cmake head/net/freerdp1/files/patch-cmake_FindOpenSSL.cmake head/net/freerdp1/files/patch-ffmpeg29 head/net/freerdp1/files/patch-git_1b663cef head/net/freerdp1/files/patch-git_434436b7 head/net/freerdp1/files/patch-libfreerdp-locale-timezone.c head/net/freerdp1/files/patch-libfreerdp_locale_timezone.c head/net/freerdp1/files/patch-winpr.pc.in head/net/freerdp1/files/patch-z001-CMakeLists.txt head/net/freerdp1/pkg-plist Committed in r428330 and r428332. Thanks for the submission! |
Created attachment 175961 [details] svn(1) diff of net/freerdp Recent builds have been failing on -CURRENT due to NEON support lacking in ARMv6. Fix this by adding a NEON option (idea boosted from multimedia/ffmpeg) that is OFF by default. Whilere here, go ahead and convert much of the _CMAKE_ON/_CMAKE_OFF options to using _CMAKE_BOOL (slightly cleaner), and pet portlint by removing extraneous spaces in pkg-plist. NOTE: Builds will still fail on release/11.0.1, stable/10, and stable/9 with errors regarding failing to resolve fmodl. It was fixed in r305380 on HEAD and MFC'd in r305971 on stable/11.