Skip to content

Linux: make server launcher container-configurable - #516

Draft
jzhvymetal wants to merge 1 commit into
google:masterfrom
opensagetv-vibe:sagetv-review/linux-server-launcher
Draft

jzhvymetal wants to merge 1 commit into
google:masterfrom
opensagetv-vibe:sagetv-review/linux-server-launcher

Conversation

@jzhvymetal

@jzhvymetal jzhvymetal commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • treat the obsolete activkey file as optional instead of emitting an error when it is absent
  • default the PID file to writable /tmp/sagetv.pid for a non-root Linux/container runtime
  • preserve administrator-provided PIDFILE, HEADLESS, JAVAMEM, and JAVAOPTS environment values
  • retain sagesettings as the final launcher override

User-visible maintenance need

The canonical launcher hard-codes /var/run/sagetv.pid and overwrites environment values before reading sagesettings. 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:

VAR_RUN_WRITABLE=false
TMP_WRITABLE=true

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/startsagecore passes.

Compatibility and risk

  • Existing root/service launches may still set PIDFILE=/var/run/sagetv.pid explicitly or through sagesettings.
  • Existing default headless mode, heap size, Java options, restart loop, arguments, and classpath are unchanged.
  • sagesettings remains authoritative when present.
  • This patch does not implement working-directory independence; that inaccurate earlier description has been removed.

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.

@google-cla

google-cla Bot commented Sep 29, 2026

Copy link
Copy Markdown

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.

@Narflex

Narflex commented Sep 29, 2026

Copy link
Copy Markdown
Collaborator

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?

@jzhvymetal

Copy link
Copy Markdown
Contributor Author

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.

@jzhvymetal

Copy link
Copy Markdown
Contributor Author

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 activkey handling, the default PID path, and environment override behavior. Those are different policy changes and were not supported by the stated reproduction. This PR should not remain open as described.

@jzhvymetal jzhvymetal closed this Sep 29, 2026
@jzhvymetal jzhvymetal changed the title Linux: make server launcher independent of working directory Linux: make server launcher container-configurable Sep 29, 2026
@jzhvymetal jzhvymetal reopened this Sep 29, 2026
@jzhvymetal
jzhvymetal marked this pull request as draft September 29, 2026 20:06
@Narflex

Narflex commented Sep 29, 2026

Copy link
Copy Markdown
Collaborator

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.

@jzhvymetal

jzhvymetal commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor Author

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:

  • Only small, independently justified changes remain ready for review.
  • Protocol, DVD-runtime, and native modernization work remains in draft until it has the required design review, reproduction evidence, or physical testing.
  • Proposals without a demonstrated user or maintenance benefit have been closed.
  • Each remaining proposal is being treated individually rather than as one large batch.

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

@Narflex

Narflex commented Sep 29, 2026

Copy link
Copy Markdown
Collaborator

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. :)

@jzhvymetal

Copy link
Copy Markdown
Contributor Author

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.

@jzhvymetal

Copy link
Copy Markdown
Contributor Author

Also originally I reached out to @jusjoken and I will share his response......

Ideally and the actual reviewer of you pull requests if you want them in the sagetv core (which I believe is the right direction) is Jeffery (Narflex). In order for these to get processed into the main core you need to fork the "https://github.com/google/sagetv" repo and then apply all your changes there and then create your series of pull requests against that main repo. There is a Google related process you will need to go through to allow your Github user account to create PRs on that Google repo but you will see that when you do your first. I find the details in your PRs great but ultimately Narflex will have to decide and review each one so you may want to do just a couple to start with to not overwhelm and then after some back and forth with Narflex as needed on the first few you can use that feedback if any to adjust the remaining PRs.

@Narflex

Narflex commented Sep 29, 2026

Copy link
Copy Markdown
Collaborator

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.

@jzhvymetal

Copy link
Copy Markdown
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants