Comment 1 Kubilay Kocak 2016-10-20 06:44:04 UTC

(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?

Comment 2 Mikael Urankar 2016-10-20 11:48:27 UTC

armv6hf is dead, you can remove it.

Comment 3 Kyle Evans 2016-10-20 13:32:29 UTC

@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.

Comment 4 Kubilay Kocak 2016-10-21 02:22:18 UTC

(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.

Comment 5 Kyle Evans 2016-10-21 03:06:30 UTC

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.

Comment 6 Kubilay Kocak 2016-10-21 03:10:05 UTC

@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

Comment 7 Kyle Evans 2016-10-21 03:13:31 UTC

(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.

Comment 8 Mikael Urankar 2016-10-21 08:45:39 UTC

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)

Comment 9 Kyle Evans 2016-10-21 12:59:46 UTC

(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.

Comment 10 Mikael Urankar 2016-10-21 15:09:14 UTC

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

Comment 11 Kyle Evans 2016-10-21 15:27:11 UTC

Created attachment 176027 [details]
Revised^2 svn(1) diff of net/freerdp

Comment 12 Kyle Evans 2016-10-21 15:29:25 UTC

(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! =)

Comment 13 Kyle Evans 2016-10-21 15:35:37 UTC

Created attachment 176028 [details]
Revised^2 svn(1) diff of net/freerdp

Comment 14 Kyle Evans 2016-10-21 15:39:57 UTC

Created attachment 176029 [details]
Revised svn(1) diff of net/freerdp

...re-read the last post.

Comment 15 Kyle Evans 2016-10-21 15:45:26 UTC

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.

Comment 16 Kyle Evans 2016-10-21 16:38:03 UTC

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 17 Jan Beich 2016-11-09 05:11:26 UTC

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'

Comment 18 Kyle Evans 2016-11-10 02:49:07 UTC

(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. =)

Comment 19 Jan Beich 2016-11-10 07:05:33 UTC

(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.

Comment 20 Kyle Evans 2016-11-14 04:23:14 UTC

(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?

Comment 21 commit-hook 2016-12-11 06:11:25 UTC

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

Comment 22 Ben Woods 2016-12-11 06:20:05 UTC

Committed in r428330 and r428332. Thanks for the submission!