Created attachment 173877 [details]
shar file

Comment 2 Kyle Evans 2016-08-19 22:11:01 UTC

(In reply to Antenore Gatta from comment #0)

Hi!

I've actually prepared an update to this exact commit already (available: https://github.com/kevans91/freebsd-ports/tree/master/net/freerdp)

I've been mulling about on officially updating this, though, given that:

1.) There's been little visible activity from upstream for a bit now, from what I can tell

2.) There's not been an official release of this branch/version

And most importantly:

3.) I've not had time to actually test out this version

However, if there's demand and this version is otherwise considered stable, then we can happily get this updated.

(In reply to Kyle Evans from comment #2)


Hi Kyle!

Thanks for the quick reply!

> 1.) There's been little visible activity from upstream for a bit now, from what I can tell
>

That's normal, it's just vacation time.

> 2.) There's not been an official release of this branch/version

This is something we are discussing since a long time and I think they won't do it at least for the next year as well...

>
> And most importantly:
>
> 3.) I've not had time to actually test out this version

I do quite often as I use FreeRDP inside Remmina, the old version are extremely bugged, now FreeRDP can be considered stable (that's why we push for a release).

The only issue could be around Vinagre, but if that happens I can check with the Gnome people if they can update their RDP APIs. 

> 
> However, if there's demand and this version is otherwise considered stable, then we can happily get this updated.

Yes, yes, yes, please! Remmina cannot work with the old freerdp version, the API is changed too much.

Taking profit of your patience and availability, I'm having an hard time to make the Remmina port working with our last tag.

How can I get support? IRC, bugzilla, ML... Many thanks in advance.

Comment 4 Kyle Evans 2016-08-20 01:03:22 UTC

(In reply to Antenore Gatta from comment #3)

Hi,

I seem to be getting some things mixed up, and even the current version wasn't really considered "stable" (the tag it points to being a beta version). As such, my current concerns are looking more and more shakey. Some facts:

Since this version:

- 433 GH issues closed -- I will not speculate on how many of these may have been legitimate bugs at this time
- 346 of those were opened after the last tagged release, leaving 87 issues predating this tag that were closed after the fact
- 292 issues opened after the last tag, not yet closed

Numbers aside, it looks like there's been some major regressions fixed since then, and I'm leaning towards 'this is a good idea'. I'll get back to you shortly on this.

If you need any other information let me know.

Comment 6 Kubilay Kocak 2016-08-20 12:19:58 UTC

Comment on attachment 173876 [details]
freerdp SVN diff

This patch removes the net/freerdp port, which I don't believe is intended.

Comment 7 Kubilay Kocak 2016-08-20 12:22:33 UTC

@Antenore Thanks for your submission. All updates to ports should be provided in unified diff (preferably svn diff) format. Can you provide a new/updated patch please

Comment 8 Kubilay Kocak 2016-08-20 12:23:06 UTC

Comment on attachment 173877 [details]
shar file

Obsolete attachment: Update is in shar format (should be svn diff)

Comment 9 Kubilay Kocak 2016-08-20 12:24:09 UTC

Antenore/Kyle, we'll also need confirmation this change passes QA (portlint, poudriere). See the following article for instructions:

https://www.freebsd.org/doc/en_US.ISO8859-1/books/porters-handbook/testing.html

Hi, 

I don't get why you deleted the unified diff patch that was already enclosed.

Moreover in my first message I wrote that the package passed portlint without warning.

Please review again and explain better what is wrong.

Comment 11 Kubilay Kocak 2016-08-20 13:13:35 UTC

(In reply to Antenore Gatta from comment #10)

In comment 6 I mentioned "This patch removes the net/freerdp port, which I don't believe is intended."

If you view the contents of attachment 173876 [details], you will see that it removes all content (files) of all port files.

Regarding the request for QA:

- portlint is only a static parser, and does not test build/packaging/etc (poudriere does)
- I only meant that QA should be *re*-run after another patch was uploaded, as it would be different from the first two.

Thanks for the explanation.

I'm quite surprised of what I've generated... :-(

Comment 13 Kyle Evans 2016-08-20 14:42:03 UTC

(In reply to Antenore Gatta from comment #12)

Hi,

No worries- I'll submit this soon. I've ran some test builds w/ openssl, but I'd like to verify that libressl builds are still fine.

Comment 14 Kyle Evans 2016-08-20 21:47:00 UTC

Created attachment 173899 [details]
Patch for net/freerdp update to 2.0.0

Apologies- it ended up being kind of a large one. The patches all had to be revisited, and a couple of them went away or a got smaller. As such, I went ahead and re-rolled them into `make makepatch` format while we're already upgrading it. Some notes pertaining to this update:

* Given that I didn't find any official declaration of version (outside of 2.0.0) from upstream, I went ahead and called this version '2.0.0a' -- assuming that we'll just increment the letter for any other intermediate releases we do, without further information from upstream.

* I went ahead and cleaned up some of the CMAKE_ usage, especially where options are concerned.

* Upstream's manpages aren't gzipped, so I did this manually in post-install -- not sure if there's a better way

* 1.2.0 had this problem as well, but this version will still fail to build on ARM. I've decided not to address this with the port, because I still have hope for bug #211965 to be pushed through/fixed. I suspect I could still add a patch to 'fix' this on non-CURRENT versions, if necessary.

I've tested building with Poudriere on 9.3 and 10.3 amd64, both using ssl=libressl and ssl=openssl. I've informally built it locally (-CURRENT) for testing as well, so that's also good to go. Will attach/link to logs on request.

Comment 15 Dmitry Marakasov 2016-08-22 10:34:06 UTC

Glancing through the patch:

- You don't need manual manpage compression, frameworks handles that, so no MANPAGE and no post-install: are needed
- PORT_OPTIONS:MGSTREAMER/PORT_OPTIONS:MX11 may be switched to options helpers, e.g. GSTREAMER_CMAKE_ON, X11_CMAKE_ON (though it also looks to me that the logic in the patch is inversed, e.g. it needs GSTREAMER_CMAKE_ON/X11_CMAKE_OFF).
- Since you're using synthetic version number, better go with 2.0.0.a, as 2.0.0.a < 2.0.0 < 2.0.0a - updating from 2.0.0.a to 2.0.0 when it's release won't require a PORTEPOCH bump

- Most importantly, since it's major version bump and include path change, it may and will probably break all dependent ports - these should be checked and bumped.

Since this is an unstable version and may affect other ports, please consider adding this as another port, e.g. net/freerdp2 or net/freerdp-devel. This will resolve potential compatibility problems, but you'll need some extra hacks to make two ports coexist - that is, rename conflicting files. It looks that these are only executable files (simple MV in post-patch will do) and libraries (strange that upstream hasn't renamed them while they should've as they have renamed include directoty and pkgconfig files). This is better to be done through CMakeLists.txt modification, as not only .so files need to be renamed but SONAME compiled in them changed.

Comment 16 Kyle Evans 2016-08-22 13:20:42 UTC

(In reply to Dmitry Marakasov from comment #15)

1.) Cool! I hadn't tried just listing it in the plist as gzip'd and letting the framework work it out on its own- that's really nifty. Thanks!

2.) Fixing

3.) Fixing

4.) Fair enough- the version bump seemed kind of arbitrary in the first place, but I do like the idea of adding it as net/freerdp-devel. I'll play around with net/vinagre a little bit just to verify, even though it won't be as crucial if this gets re-introduced as net/freerdp-devel. The other one (net/remmina-plugin-rdp) is BROKEN and presumably what Antenore aims to address after this update.

What somewhat bothers me, though, is that I'm afraid we'll get in a state where net/freerdp will never actually be updated given the sparse upstream releases, the last couple of which are beta releases from 2 and 3 years ago. Then, we'll eventually (and possibly in the not-so-distant future) get to a point where it makes more sense to just replace net/freerdp with net/freerdp-devel.

Thanks to both for your support, I'm honestly more than lost (and afraid for remmina, 'cos I don't know how to proceed).

Regarding your last comments I confirm everything, especially the last one.

Comment 18 Kyle Evans 2016-09-20 13:15:25 UTC

Hi,

FYI- I've not forgotten/ignored this. I intend to finish this up soon-ish, and introduce it as net/freerdp2. The new port will be providing libfreerdp2{,-client}.so, , libwinpr2{,-tools}.so, along with {d,x}freerdp2 and winpr2-{hash,makecert}.

Thanks,

Kyle Evans

Thenks Kyle! Sounds good to me :-)

Comment 20 Kyle Evans 2016-09-22 05:41:18 UTC

(In reply to Antenore Gatta from comment #19)

Hi,

I believe I've got this all settled out- see https://github.com/kevans91/freebsd-ports/tree/master/net/freerdp2 if you're curious and/or want something to work with. Unlike what I said earlier, the *.so's provided are libfreerdp-client2.so, libfreerdp2.so, libwinpr-tools2.so, and libwinpr2.so.

Worth noting is that I'm going to go ahead and drop the DIRECTFB option in net/freerdp2. This option doesn't compile as-is, and it is almost completely unmaintained upstream.

I'm still working on slimming down the number of patches, but following that I'll submit this as a new port.

Kyle, I've tested it, it's really neat and freerdp works.

I don't have time to compile remmina against this freerdp version, because I have to modify the cmake files, but I expect it'll work fine.

Thanks!

Comment 22 Kyle Evans 2016-09-28 03:37:30 UTC

Created attachment 175216 [details]
shar(1) archive of proposed net/freerdp2

@amdmi3: I'm attaching my proposed net/freerdp2 port. I believe I've got the number of patches down to an acceptable number -- the remaining patches are either non-trivial or don't have enough in common, in my opinion, to bother replacing them with a ${REINPLACE_CMD} -e. Everything still builds well, and I've ripped out the DFREERDP option because the dfreerdp client remains unmaintained and it's been noted as such for a while now in net/freerdp -- I think it's time.

Comment 23 Ben Woods 2016-11-24 15:19:01 UTC

Hi Kyle,

I am looking at committing this update to net/freerdp for you, but I believe it is better to not create a new port net/freerdp2, and instead ensure all ports which depend on it work with the new version, or are patched to work with it. E.g. When I looked at this in June, vinagre had to also be patched to support the new freerdp (see my patch attached to PR198618).

As you have stated, it has been a long time between drinks when it comes to freerdp having official releases, however there has been so much progress in the code that using the official releases barely makes sense anymore.

I have made a couple of changes to your diff to incorporate some of the changes I made in my patch in PR198618. I'll attach it soon for your review/approval.

Comment 24 Ben Woods 2016-11-24 15:19:15 UTC

*** Bug 213905 has been marked as a duplicate of this bug. ***

Comment 25 Ben Woods 2016-11-24 15:19:32 UTC

*** Bug 198618 has been marked as a duplicate of this bug. ***

Please, note also that the FreeRDP project is changing its API again and quite a lot. We, the Remmina project, are in the process of adapting the Remmina code to work with the new FreeRDP and helping them (Giovanni is doing it), to fix some seg fault.

I honestly don't know what is better to do. FreeRDP is not yet stable and per consequence most of the software using it aren't stable as well (remmina, vinagre). 

This is the last FreeRDP commit that can be considered """stable""":

https://github.com/FreeRDP/FreeRDP/commit/1d06087b601a4cc723592e9a6efbf9bee0dc2d12

If I can do anything let me know and thanks so much for your support.

Comment 27 Kyle Evans 2016-11-24 17:21:36 UTC

(In reply to Ben Woods from comment #23)

Hi Ben,

Thanks! We should go ahead and incorporate some of the changes/decisions from PR 213637 at that time, then, w.r.t NEON option and BROKEN=. The patch attached to it mostly goes away, but we should:

1. Pull in the NEON option and BROKEN= bits
2. Fix the NEON_CFLAGS to only add -march=armv7-a if we're missing -march, as per jbeich@ comment 20
3. Add the -D__ARM_NEON__ for aarch64, also as per jbeich@ comment 20.

Comment 28 Ben Woods 2016-11-24 17:24:31 UTC

Created attachment 177362 [details]
Patch to update net/freerdp to 2.0.0 as of 2016.11.24, and patch net/vinagre

The attached patch builds on the one provided by Kyle, but updates net/freerdp to version 2.0.0 as of 2016.11.24, rather than creating a new net/freerdp2 port.

This relies on the ports which depend on net/freerdp being patched to work with 2.0.0.

With this patch the build of net/vinagre is still failing with the following error:
plugins/rdp/vinagre-rdp-tab.c:585:21: error: use of undeclared identifier 'CLRBUF_32BPP'

net/guacamole-server also fails to build, as it does not find the freerdp components with its configure script. This will require patching also.

Whilst this patch needs work to fix this vinagre build error, it would be good if we could work collaboratively on this patch. Please use this as the latest for any future works, and upload any improvements to this patch as they are made.

Attachment 177362 [details] doesn't apply ...

Created attachment 177457 [details]
Patch to update net/vinagre to 3.22.0

Instead of patching vinagre, I think we can update it to the latest version. However, vinagre 3.22.0 works with commit 1855e36 (the version used by Fedora) but not 2a6dbab.

I added a patch from https://bugzilla.gnome.org/show_bug.cgi?id=765444 to allow it to use freerdp2.pc. yelp-tools is added to BUILD_DEPENDS in order to run autoreconf. This patch is probably not ready to be reviewed or committed. I only use it to test whether vinagre build with specific version of freerdp. I have not tested it in poudriere.

Comment 31 Kyle Evans 2016-11-28 04:41:54 UTC

Comment on attachment 177362 [details]
Patch to update net/freerdp to 2.0.0 as of 2016.11.24, and patch net/vinagre

Hi,

woodsb02@- As Ting-Wei points out, this patch does not actually apply as-is -- several patches that appear to be renamed outside of svn and then patched, leaving an awkward result where it removes the patches that got renamed and then tries to apply the patches to the new names, then fails because the diff implies those segments are patches to an existing file than creating a new file.

Unfortunately, I don't quite have the mental capacity to straighten it out tonight. =) I'll take a peek tomorrow or the day after, fix up this patch (if you hadn't already by then), and add in the missing BROKEN= and some of the other NEON stuff that didn't quite make it into a patch.

Comment 32 Ben Woods 2016-11-30 15:06:18 UTC

Created attachment 177540 [details]
Patch to update net/freerdp to 2.0.0 as of 2016.11.24

(In reply to Ting-Wei Lan from comment #29)
My patch didn't apply because I am using "svn move" to rename files to the new patch naming convention (pet portlint), but it then "svn diff" doesn't create a patch that applies cleanly.

This new patch was generated with "svn diff --no-diff-deleted", and should apply with the following sequence of commands:

$ cd /usr/ports/net/freerdp/
$ svn mv files/patch-z001-CMakeLists.txt files/patch-CMakeLists.txt
$ svn mv files/patch-cmake_FindGStreamer_1_0.cmake files/patch-cmake_FindGStreamer__1__0.cmake
$ svn mv files/patch-freerdp.pc.in files/patch-libfreerdp_freerdp.pc.in
$ svn mv files/patch-winpr.pc.in files/patch-winpr_winpr.pc.in
$ svn patch /PATH/TO/DOWNLOADED/PATCH

To clarify, you should then see the following changes in the net/freerdp directory:

$ svn status
M       Makefile
M       distinfo
A  +    files/patch-CMakeLists.txt
        > moved from files/patch-z001-CMakeLists.txt
D       files/patch-cmake-FindOpenSSL.cmake
D       files/patch-cmake_ConfigOptions.cmake
D       files/patch-cmake_FindGStreamer_1_0.cmake
        > moved to files/patch-cmake_FindGStreamer__1__0.cmake
A  +    files/patch-cmake_FindGStreamer__1__0.cmake
        > moved from files/patch-cmake_FindGStreamer_1_0.cmake
M       files/patch-ffmpeg29
D       files/patch-freerdp.pc.in
        > moved to files/patch-libfreerdp_freerdp.pc.in
D       files/patch-git_1b663cef
D       files/patch-git_434436b7
D       files/patch-libfreerdp-locale-timezone.c
A  +    files/patch-libfreerdp_freerdp.pc.in
        > moved from files/patch-freerdp.pc.in
D       files/patch-winpr.pc.in
        > moved to files/patch-winpr_winpr.pc.in
A  +    files/patch-winpr_winpr.pc.in
        > moved from files/patch-winpr.pc.in
D       files/patch-z001-CMakeLists.txt
        > moved to files/patch-CMakeLists.txt
M       pkg-plist

Comment 33 Ben Woods 2016-11-30 15:08:38 UTC

Comment on attachment 177457 [details]
Patch to update net/vinagre to 3.22.0

Does the FreeBSD GNOME team see any issue with upgrading net/vinagre to 3.22, which is a more recent version than GNOME itself?

Created attachment 177608 [details]
Patch to update net/vinagre to 3.22.0

Updated patch that seems to work with freerdp 2016.11.24. I will submit it to upstream for reviewing.

Comment 35 Ben Woods 2016-12-03 00:44:57 UTC

(In reply to Ting-Wei Lan from comment #34)
Hi Ting-Wei Lan,
Is it possible that this patch (or similar) could be applied to vinagre 3.18 to allow it to work with freerdp2 2016.11.24?
I ask because I believe it is normal for vinagre to be updated to new versions in line with the entire gnome desktop, which remains at version 3.18 in ports for now.
Thoughts?

(In reply to Ben Woods from comment #35)
I tried to keep PORTVERSION at 3.18.2. All patches in files directory could be successfully applied, but vinagre could not be built:

plugins/rdp/vinagre-rdp-tab.c:1127:13: error: no member named 'DisableEncryption' in 'struct rdp_settings'
  settings->DisableEncryption = FALSE;
  ~~~~~~~~  ^
7 warnings and 1 error generated.

Comment 37 Kyle Evans 2016-12-03 17:55:05 UTC

Created attachment 177636 [details]
svn(1) diff, makefile only

Hi woodsb02@,

After reviewing your proposed patch, it all looks good- but I counter-propose the attached patch in place of your Makefile patch. I applied yours, then made the following changes:

1.) Added back in the BROKEN=, so that users of affected versions (to include 11.0-RELEASE) are notified.
2.) Removed redundant xcursor in X11_USE (reported by z7dr6ut7gs@snkmail.com, PR 214956)
3.) Added in jbeich@'s recommended aarch64 NEON bits

If this looks good, I'd also like to ping jbeich@ and mikael.urankar@gmail.com so that they can take a peek at the revised form and make sure it still looks OK to them if they wish to.

Comment 38 Ben Woods 2016-12-04 16:34:48 UTC

I have uploaded this for review on phabricator:
https://reviews.freebsd.org/D8712

Please make any final comments/approval there.

I submitted my vinagre patch to upstream:
https://bugzilla.gnome.org/show_bug.cgi?id=775616

I've been using freerdp as built with this patch for a week with success (connecting to a win7 host).

Comment 41 commit-hook 2016-12-11 04:38:01 UTC

A commit references this bug:

Author: woodsb02
Date: Sun Dec 11 04:36:59 UTC 2016
New revision: 428330
URL: https://svnweb.freebsd.org/changeset/ports/428330

Log:
  net/freerdp: Update to 2.0.0 pre-release (GitHub as of 2016.11.24)

  - This update brings in the latest 2 years of FreeRDP project work,
    which has had a long time between tagging releases.
  - 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
  - Fix issue with X11_USE=xorg= being truncated due to whitespace as it
    wrapped over multiple lines
  - Move installed *.cmake files to correct location for modules

  net/freerdp1:
  - create new port based on previous net/freerdp 1.2.0, as it is required
    by net/guacamole-server

  net/vinagre:
  - patch to work with the new version of net/freerdp 2.0.0

  net/guacamole-server:
  - patch to work with net/freerdp1

  PR:		212004
  PR:		214956
  Submitted by:	Kyle Evans (maintainer)
  Reported by:	John Hein <z7dr6ut7gs@snkmail.com>
  Reviewed by:	Mikael Urankar <mikael.urankar@gmail.com>
  Reviewed by:	Ting-Wei Lan <lantw44@gmail.com>
  Reviewed by:	Antenore Gatta <antenore@simbiosi.org>
  Reviewed by:	amdmi3
  Approved by:	adamw (mentor)
  Differential Revision:	https://reviews.freebsd.org/D8712

Changes:
  head/net/Makefile
  head/net/freerdp/Makefile
  head/net/freerdp/distinfo
  head/net/freerdp/files/patch-CMakeLists.txt
  head/net/freerdp/files/patch-cmake-FindOpenSSL.cmake
  head/net/freerdp/files/patch-cmake_ConfigOptions.cmake
  head/net/freerdp/files/patch-cmake_FindGStreamer_1_0.cmake
  head/net/freerdp/files/patch-cmake_FindGStreamer__1__0.cmake
  head/net/freerdp/files/patch-ffmpeg29
  head/net/freerdp/files/patch-freerdp.pc.in
  head/net/freerdp/files/patch-git_1b663cef
  head/net/freerdp/files/patch-git_434436b7
  head/net/freerdp/files/patch-libfreerdp-locale-timezone.c
  head/net/freerdp/files/patch-libfreerdp_freerdp.pc.in
  head/net/freerdp/files/patch-winpr.pc.in
  head/net/freerdp/files/patch-winpr_winpr.pc.in
  head/net/freerdp/files/patch-z001-CMakeLists.txt
  head/net/freerdp/pkg-plist
  head/net/freerdp1/
  head/net/freerdp1/Makefile
  head/net/freerdp1/files/patch-client_X11_CMakeLists.txt
  head/net/freerdp1/files/patch-client_X11_ModuleOptions.cmake
  head/net/freerdp1/files/patch-client_X11_cli_CMakeLists.txt
  head/net/freerdp1/files/patch-client_X11_xfreerdp.1.xml.in
  head/net/freerdp1/files/patch-client_common_CMakeLists.txt
  head/net/freerdp1/files/patch-freerdp.pc.in
  head/net/freerdp1/files/patch-libfreerdp_CMakeLists.txt
  head/net/freerdp1/files/patch-winpr_tools_hash_CMakeLists.txt
  head/net/freerdp1/files/patch-winpr_tools_makecert_CMakeLists.txt
  head/net/freerdp1/pkg-plist
  head/net/guacamole-server/Makefile
  head/net/guacamole-server/files/patch-configure.ac
  head/net/vinagre/Makefile
  head/net/vinagre/files/patch-configure.ac
  head/net/vinagre/files/patch-plugins_rdp_vinagre-rdp-tab.c

Comment 42 Ben Woods 2016-12-11 04:40:49 UTC

Committed, thanks for everyone's work on this - it was a long time in the making.

Comment 43 commit-hook 2016-12-11 06:11:22 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