Bug 212004
| Summary: | net/freerdp: Update to 2.0.0.d | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | Ports & Packages | Reporter: | Antenore Gatta <antenore> | ||||||||||||||||||||
| Component: | Individual Port(s) | Assignee: | Ben Woods <woodsb02> | ||||||||||||||||||||
| Status: | Closed FIXED | ||||||||||||||||||||||
| Severity: | Affects Only Me | CC: | gnome, jcfyecrayz, kevans, koobs, lantw44, rozhuk.im, woodsb02 | ||||||||||||||||||||
| Priority: | --- | Keywords: | needs-qa, patch | ||||||||||||||||||||
| Version: | Latest | Flags: | bugzilla:
maintainer-feedback?
(kevans) |
||||||||||||||||||||
| Hardware: | Any | ||||||||||||||||||||||
| OS: | Any | ||||||||||||||||||||||
| URL: | https://reviews.freebsd.org/D8712 | ||||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||||
| Bug Blocks: | 213637, 214956 | ||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||
Created attachment 173877 [details]
shar file
(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. (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 on attachment 173876 [details]
freerdp SVN diff
This patch removes the net/freerdp port, which I don't believe is intended.
@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 on attachment 173877 [details]
shar file
Obsolete attachment: Update is in shar format (should be svn diff)
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. (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... :-( (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. 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. 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. (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. 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 :-) (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! 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.
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. *** Bug 213905 has been marked as a duplicate of this bug. *** *** 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. (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. 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 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.
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 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.
(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. 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. 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). 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 Committed, thanks for everyone's work on this - it was a long time in the making. 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 |
Created attachment 173876 [details] freerdp SVN diff I work in the FreeRDP group as the Remmina maintainer. This patch update FreeRDP to one of the last commit, without this version Remmina cannot work properly. This is my first attempt to update a port so things may be not perfect, but all the tests passed without issues, also using the portlint. The only impact I know could be with Vinagre, but I'm not into it, so I really cannot know. Thanks in advance for your precious validation.