GUACAMOLE-249: Migrate to FreeRDP 2.x #243
Conversation
…. If accepting the certificate, request that FreeRDP not store it.
…mats. The "CB_FORMAT_*" constants which used to be defined by FreeRDP no longer exist.
…tents of rdpGdi struct.
…d and renamed to "UseRdpSecurityLayer".
…t those that must be overridden.
… migration (commit a5b62aa).
…andlers will otherwise fail).
Most plugins built into FreeRDP implement the PVIRTUALCHANNELENTRYEX entry point, but the FreeRDP standard function for loading plugins only supports PVIRTUALCHANNELENTRY. It appears that only the commandline argument parser included with FreeRDP was updated to leverage the new entry points.
The CLIPRDR specification requires that the msgFlags field for the Format List PDU be set to 0x0000. The function within FreeRDP overrides this value to 0x0000, but it is still incorrect to attempt to set it.
…ssing a Format List PDU.
|
After fixing the RAIL issue, I encountered the following: Apparently, the Adding tests to determine the proper function doesn't appear to be an option at the moment, as:
I tried addressing this by switching to In this case, the incompatibility is due to switching from I've also created some Jenkins jobs to verify the build, so we should be able to better ensure build compatibility going forward:
|
mike-jumper
force-pushed
the
freerdp-migrate-2.x
branch
from
6dde3a1
to
4282da6
Compare
|
Rerunning builds for all FreeRDP tags, it looks like the current code will build only against 2.0.0-rc4. Earlier tags suffer from the issues noted above for Bionic. |
Lovely...so we're tied to a current RC of FreeRDP 2. That feels safe. |
Well, 2.0.0-rc4 and master, but yeah - I know what you mean. I'm not necessarily against trying to be compatible with rc0 through rc3, but I'm not sure how we would distinguish them. It doesn't seem possible to write configure tests because of pkg-config, and the version numbers are identical, even within FreeRDP's version.h. 2.0.0-rc0#define FREERDP_VERSION_MAJOR 2
#define FREERDP_VERSION_MINOR 0
#define FREERDP_VERSION_REVISION 0
#define FREERDP_VERSION_SUFFIX "dev5"
#define FREERDP_API_VERSION "2"
#define FREERDP_VERSION "2.0.0"
#define FREERDP_VERSION_FULL "2.0.0-dev5"
#define GIT_REVISION "326a44895"2.0.0-rc4#define FREERDP_VERSION_MAJOR 2
#define FREERDP_VERSION_MINOR 0
#define FREERDP_VERSION_REVISION 0
#define FREERDP_VERSION_SUFFIX "dev5"
#define FREERDP_API_VERSION "2"
#define FREERDP_VERSION "2.0.0"
#define FREERDP_VERSION_FULL "2.0.0-dev5"
#define GIT_REVISION "326a44895" |
|
I'll try nested autoconf. Perhaps adding a configure script which is dedicated to RDP and which is invoked by the top-level configure script will allow us to perform tests which inherit flags from pkg-config. |
|
aHA! Manually setting The downside is that it adds the flags globally. It shouldn't hurt the build, as part of the reason this is so cumbersome is that FreeRDP 2.x is double-namespaced, with headers being within |
…regardless of whether const is required.
…nvertColor(), but was only available as ConvertColor() in older FreeRDP 2.0.0 release candidates.
|
OK, I'm having success with configure tests and am rewriting the previous typecast solutions for CLIPRDR, RAIL, etc. to instead use tests. This should be more future-proof in the sense that we'll get obvious build failures if API compatibility breaks, and it should allow us to support FreeRDP: Bionic Edition as well as 2.0.0-rc0 onward. |
…-rc3 and older (used UINT32 instead of INT32).
…configure tests to check need for const.
…figure tests to check need for const.
…reeRDP tests affect only FreeRDP tests.
…sts fail when pointers are incompatible.
|
Everything looks good with CentOS7 with 2.0.0-rc4 |
|
OK, I've added configure tests and things seem to build across the board now: git master, 2.0.0-rc0 through 2.0.0-rc4, as well as Bionic Edition. The ASF Jenkins instance is too backed up at the moment to be used to verify the builds, but I'm doing so manually on my own dev machine via Docker. |
|
I've manually confirmed on:
I think we're good to go. |
|
Sounds good. Giving @jmuehlner one last chance to throw in any input - otherwise I'll plan to merge it later today. You mentioned needing to deal with conflicts in the master - anything special that we need to worry about for that? |
Only that these conflicts are likely to be more difficult to resolve than the usual conflicts. I've tried to summarize the specific issues that conflicted and how to resolve in the issue description. AFAIK, there's no real way to provide a patch which addresses merge conflicts, but I went ahead and performed the merge myself as a test, manually resolving the conflicts, and have been cherry-picking any new commits to that branch since then: https://github.com/mike-jumper/guacamole-server/tree/freerdp-migrate-2.x-resolve-merge-master My hope is that the above branch might help in verifying the result of the merge and/or in guiding the resolution of the conflicts. |
|
Sounds good...I think it's go time...
|
|
Okay, merge to |
|
Merge to |
|
Looks real solid. I just manually tried merging the previous I think that log message should probably be removed, and I will open a PR to that effect in a moment. |
This change refactors the entirety of Guacamole's RDP support to use FreeRDP 2.x, removing any remaining compatibility with older FreeRDP releases. These changes also:
There are going to be conflicts with master for the following non-1.1.0 changes:
int->uint64_tchange has to be reapplied manually (see below).To hopefully ease this, I've performed a test merge and manually resolved the above conflicts. I recommend resolving conflicts such that things match: https://github.com/mike-jumper/guacamole-server/tree/freerdp-migrate-2.x-resolve-merge-master
... Good luck.