Fix: #294 Support XRDP backend#491
Conversation
|
Any chance this can be merged, or is the project dead? |
|
Hi, project is basically dead. :( Maintainer changed job so there is no one to maintain anymore cc @obilodeau |
|
That's a shame. 😞 |
|
@yaleman we don't have a good test harness with many clients and servers. I basically need a bunch of VMs to make sure that this change doesn't cause regression anywhere else. I could trust you and merge to keep things moving but I would like some guarantees. Did you test with Windows desktop 10/11 as a target and did you test with Windows Server as a target? Thanks! |
|
I've tested it with Windows 10 and a Windows 2022 server and as far as I can tell it works. Had to add some build-time dependencies in the Dockerfile so it'd build |
obilodeau
left a comment
obilodeau
left a comment
There was a problem hiding this comment.
Please revert the docker changes and submit a separate pull request. Check other docker optimization related pull requests to get a sense of what is the proper fix. I don't have time to guide you now.
Thanks!
| build-essential python3-dev pkg-config \ | ||
| libavformat-dev libavcodec-dev libavdevice-dev libavfilter-dev \ | ||
| libswscale-dev libavutil-dev \ | ||
| # minimize image size | ||
| && rm -rf /var/lib/apt/lists/* |
There was a problem hiding this comment.
Please keep these changes separate... Lots of care goes into producing a minimally sized build and leveraging PyAV pre-built wheels instead of pulling all these dependencies reduce build time and image size.
There was a problem hiding this comment.
it won't build without them, so I couldn't build/run it in that case.
There was a problem hiding this comment.
Also they're not in the final image either way - they're in the compile phase?
There was a problem hiding this comment.
However, the dependencies also need to be installed on the CI, which is why it is failing on your PR.
There was a problem hiding this comment.
I would have loved to keep track of a PyAV version that is distributed as a binary wheel (no need to build) but they are removing old versions quickly...
There was a problem hiding this comment.
I am trying to fix the CI here but it is not going smooth: #512
There was a problem hiding this comment.
Ok, now it looks good. Once all jobs pass, I'll merge and then you can rebase on main here so I see it pass with your new code. Thanks for the patience.
There was a problem hiding this comment.
#512 was merged. Please rebase, test again and hopefully the CI will pass.
|
Thanks for your patience! |
3 participants
This takes the comments from #294 and makes it work as far as I can tell.