Repository navigation
Linux: make server launcher container-configurable - #516
jzhvymetal wants to merge 1 commit into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
This is going to be a general comment at first regarding all the pull requests you submitted. Once that is sorted out, then I'll start to review relevant ones. Overall, SageTV is in a maintenance state right now and my main concern is maintaining stability of it and not introducing any new breakage. When there are problems users are actually experiencing (like with Schedules Direct), then this warrants code changes to resolve those issues. However, it appears that many of these pull requests are from an AI analysis of the codebase and it finding things that may not be optimal or may have some obscure case where a bug could be triggered and then creating a solution for that theoretical case. These changes wouldn't actually provide any user benefit, since they are not actually fixing a problem that users are hitting, but they do risk creating a new problem because there's a possibility for some case to occur that would only be uncovered with testing. The main point being, that the nil benefit for fixing a problem that nobody is having is outweighed by the risk of creating a new unknown problem. That greatly disinclines me to merge changes that don't actually fix a problem that users are experiencing. Thoughts on that? |
|
I agree with the maintenance-first concern. AI-assisted analysis and passing tests are not, by themselves, sufficient reasons to change stable Core code. I audited every submitted PR again against its actual diff and the physical/reproduction evidence, and I found that the original batch was too broad. I am reducing the review request as follows:
In particular, #516's patch does not implement the working-directory behavior described by its title/body, and #518's upstream base already sends discovery replies through the receiving socket. Those should not remain open as presented. I will treat AI as an implementation and test aid, not as evidence of user benefit. Any future upstream request will include a concrete affected behavior or maintenance need, repeatable reproduction, before/after evidence, compatibility risk, and the smallest practical patch. Anything without that evidence will remain outside the merge queue. I do not expect the draft capability/DVD/native topics to be reviewed unless and until their prerequisites and real evidence are complete and there is upstream interest in the design. |
|
Closing after maintenance triage. The title/body says this makes the launcher independent of its working directory, but the patch does not resolve or change the working directory. It instead changes legacy |
|
Thanks so much for your understanding on this. I do appreciate the work you're doing, and I'm sure many in the community will as well. |
I agree with many of those concerns. My original goal was much narrower: I wanted to get the main SageTV server compiling and running in a current Linux environment so SageTV could use current GPU drivers and modern hardware-accelerated video support. As testing continued, AI-assisted analysis identified additional areas that appeared broken or unreliable, and the scope expanded too far. I have reviewed the proposed changes again with SageTV’s maintenance status in mind. I agree that static analysis or an AI-generated test is not, by itself, sufficient justification for changing stable Core code. I have moved as much functionality as possible out of Sage.jar. Testing and commissioning controls now use a stock-compatible SageTV Standard plugin built on the existing public SageTV APIs. An external MCP adapter uses that plugin to control the server and client during repeatable tests—for example, starting playback, seeking, changing channels, selecting captions, and checking the resulting state. The purpose of that system is to reproduce reported problems and validate fixes consistently. It is not intended to justify Core changes simply because AI-assisted analysis identifies a theoretical issue. I have also reduced and reclassified the existing pull requests:
The current-Ubuntu work remains an important upstream maintenance goal. I want both the canonical SageTV server and the Vibe server—not only the Vibe container—to build and run on a current Ubuntu base with current GPU drivers. The Core changes currently associated with that goal are:
The broader #528 and #529 work still needs to be split into smaller, independently reviewable changes. After those Core changes are approved and validated with an unmodified stock Core payload, I plan to propose a separate current-Ubuntu runtime image to OpenSageTV/sagetv-dockers. That container proposal will own the runtime packages, non-root entrypoint, supervisor, health checks, GPU device configuration, and software fallback behavior. The Linux Docker container is still Linux-only. The related SageTV Core and FFmpeg plugin components are intended to support both Linux and Windows where applicable. Most of the remaining DVD-related changes came from problems reproduced during actual playback testing. DVD playback works in SageTV |
|
You're also free to fork SageTV of course and then pull in upstream changes as needed (which should be pretty minimal, possibly even never again)....or you could do it as part of OpenSageTV if the maintainers of that agree with it. Then you can be sure all of the work gets merged and it also takes the work of reviewing it off my plate. :) |
That is actually what I originally did with opensagetv-vibe-core. However, I realized I was starting to make more changes, which could eventually result in two separate branches if OpenSageTV is updated. I would like to avoid that situation because I do not want to maintain a separate fork and continually have to merge or rebase it whenever the main OpenSageTV branch is updated. My preference is to keep as much as possible compatible with the main OpenSageTV codebase. |
|
Also originally I reached out to @jusjoken and I will share his response......
|
|
Correct me if I'm wrong, but AFAIK, the OpenSageTV project simply tracks the SageTV project and doesn't have any divergent commits (and that looks to be correct, it has 1028 commits vs. 1058 in SageTV and it is currently 30 commits behind master). It was created because people weren't sure how much the community would have control over a Google owned project, but that turned out to not be a problem (I'm still a maintainer and left Google over a year ago). So the OpenSageTV project could be a good place to host it....or in you're own forked project. You shouldn't have any real problems with divergence IMHO....but it's up to you of course. |
|
That might be the case. I only recently started vibe coding it, so I'm not adware of the proper process. My original goal was simply to update the Linux Docker environment so the standard SageTV Core could run with current GPU drivers and hardware transcoding. Some of the additional changes came from gated and regression testing, where more consistent behavior was needed to make the testing repeatable and reliable. I also don’t want to maintain a separate fork. I’d rather keep everything as close to the standard Core as possible and only make the changes needed to keep it running properly on a modern system. |
Summary
activkeyfile as optional instead of emitting an error when it is absent/tmp/sagetv.pidfor a non-root Linux/container runtimePIDFILE,HEADLESS,JAVAMEM, andJAVAOPTSenvironment valuessagesettingsas the final launcher overrideUser-visible maintenance need
The canonical launcher hard-codes
/var/run/sagetv.pidand overwrites environment values before readingsagesettings. A current least-privilege container user cannot write/var/run, and container orchestrators conventionally provide JVM/runtime settings as environment variables. This leaves the server process without its expected PID file and prevents normal environment-based configuration unless the image rewrites the installed launcher or generates a settings file.This is required for a stock/canonical SageTV server payload to run cleanly in the same current-Ubuntu, non-root container model used by the Vibe image. It is not a Vibe-only feature.
Reproduction and evidence
In the Ubuntu 26 development container, running as the non-root runtime identity reports:
Source inspection shows the old launcher unconditionally assigns all four runtime variables; the patch uses shell defaults so an explicit administrator value survives.
bash -n build/serverfiles/startsagecorepasses.Compatibility and risk
PIDFILE=/var/run/sagetv.pidexplicitly or throughsagesettings.sagesettingsremains authoritative when present.Review state
Draft until the focused launcher regression harness proves default and overridden values for root and non-root launches. The change is intended for canonical SageTV once that narrow gate is attached.
Current-Ubuntu container dependency
This is one of four independent Core approvals needed before proposing the current-Ubuntu runtime image to
OpenSageTV/sagetv-dockers: #516 launcher behavior, #519 reproducible source-clean builds, #528 current GCC/64-bit native compatibility, and the separable Ubuntu/ImageLoader portions of #529. The later container PR will add the runtime image, supervisor, packages, health checks, and GPU device documentation; none of those deployment policies are part of this Core patch.