Allow processes to be started on start/stop of a session. #2138

Chris3773 · 2024-02-13T01:34:24Z

Description

Allows for processes to be started when a client connects, resumes an app or disconnects from an app.

Added new session envs.
SUNSHINE_CLIENT_ID - Not sure how useful this one is with how it's used right now.
SUNSHINE_CLIENT_UNIQUE_ID
SUNSHINE_CLIENT_SLOT - Can only be used when set to session.

I'm open to suggests for the header text for the config as I don't know if users will know what it means by Session.

Issues:

The envs SUNSHINE_APP_ID and SUNSHINE_APP_NAME can't be used when the process is set to session.
Output from the session process isn't logged.

Screenshot

Issues Fixed or Closed

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Dependency update (updates to dependencies)
Documentation update (changes to documentation)
Repository update (changes to repository files, e.g. .github/...)

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated the in code docstring/documentation-blocks for new or existing methods/components

Branch Updates

LizardByte requires that branches be up-to-date before merging. This means that after any PR is merged, this branch
must be updated before it can be merged. You must also
Allow edits from maintainers.

I want maintainers to keep my branch updated

CLAassistant · 2024-02-13T01:34:37Z

All committers have signed the CLA.

Hazer · 2024-05-12T02:53:22Z

I have some points I'd like to discuss here:

Given how sessions are used on the clients right now, this change feels confusing, because for the end-user, the session Starts once (process start and streaming start), and they can Resume (streaming start) or Quit it (streaming stop and process stop), and "Pausing/Getting out" the session (streaming stop) is kinda implicit, so the streaming session that starts and stops with each client connection, it's not so transparent to the user, at least in my point of view. Naming it per connection may make more sense, but I'm not so sure too.
How does this should handle multiple connected clients to the same Application Session? Given I connect my tablet and resume the same session on my phone, both will run the same start command, and then both will run the stop? Only the first connected client should run the starting commands and the last connected should run the stop commands? If it's the former, can you point out some use cases for me so I can test the behavior?
I think we should split Command preparations in the UI in 2 sections, "Per-Application", "Per-Client/Device" instead of using the Run on session checkbox to differentiate, this may avoid the session confusion, and allows to have a proper Title and Subtitle describing each usage and expected behavior.

Hazer · 2024-05-12T01:24:34Z

src/rtsp.h

+#include <boost/process.hpp>
+


I don't think this is needed

Hazer · 2024-05-12T01:25:09Z

src/rtsp.h

+    boost::process::environment env;
+


This seems unused, remove?

Hazer · 2024-05-12T02:56:55Z

src/stream.cpp

+      std::error_code ec;
+      std::vector<cmd_t>::const_iterator prep_it = std::end(config::sunshine.prep_cmds);
+      for (; prep_it != std::begin(config::sunshine.prep_cmds); --prep_it) {
+        auto &cmd = *(prep_it - 1);
+
+        // Skip empty and non session commands
+        if (cmd.undo_cmd.empty() || !cmd.on_session) {
+          continue;
+        }
+
+        boost::filesystem::path working_dir = find_working_directory(cmd.do_cmd);
+        BOOST_LOG(info) << "Executing Undo Cmd: ["sv << cmd.undo_cmd << ']';
+        auto child = platf::run_command(cmd.elevated, true, cmd.undo_cmd, working_dir, session.env, nullptr, ec, nullptr);
+
+        if (ec) {
+          BOOST_LOG(warning) << "Couldn't run ["sv << cmd.undo_cmd << "]: "sv << ec.message();
+        }
+
+        child.wait();
+        auto ret = child.exit_code();
+
+        if (ret != 0) {
+          BOOST_LOG(warning) << '[' << cmd.undo_cmd << "] failed with code ["sv << ret << ']';
+        }
+      }


This files looks awfully big already and this feels out of place, can we move this to another file, maybe reuse some of it in process too?

I'd suggest moving this to something like: user_commands::run_session_stop() and call it here

Hazer · 2024-05-12T02:57:20Z

src/stream.cpp

+      std::error_code ec;
+      std::vector<cmd_t>::const_iterator prep_it = std::begin(config::sunshine.prep_cmds);
+      for (; prep_it != std::end(config::sunshine.prep_cmds); ++prep_it) {
+        auto &cmd = *prep_it;
+
+        // Skip empty and non session commands
+        if (cmd.do_cmd.empty() || !cmd.on_session) {
+          continue;
+        }
+
+        boost::filesystem::path working_dir = find_working_directory(cmd.do_cmd);
+        BOOST_LOG(info) << "Executing Do Cmd: ["sv << cmd.do_cmd << ']';
+        auto child = platf::run_command(cmd.elevated, true, cmd.do_cmd, working_dir, session.env, nullptr, ec, nullptr);
+
+        if (ec) {
+          BOOST_LOG(error) << "Couldn't run ["sv << cmd.do_cmd << "]: System: "sv << ec.message();
+          return -1;
+        }
+
+        child.wait();
+        auto ret = child.exit_code();
+        if (ret != 0) {
+          BOOST_LOG(error) << '[' << cmd.do_cmd << "] failed with code ["sv << ret << ']';
+          return -1;
+        }
+      }


I'd suggest moving this to something like: user_commands::run_session_start() and call it here

Hazer · 2024-05-12T03:02:31Z

src/stream.cpp

+    boost::filesystem::path
+    find_working_directory(const std::string &cmd) {
+      // Parse the raw command string into parts to get the actual command portion
+#ifdef _WIN32
+      auto parts = boost::program_options::split_winmain(cmd);
+#else
+      auto parts = boost::program_options::split_unix(cmd);
+#endif
+      if (parts.empty()) {
+        BOOST_LOG(error) << "Unable to parse command: "sv << cmd;
+        return boost::filesystem::path();
+      }
+
+      BOOST_LOG(debug) << "Parsed executable ["sv << parts.at(0) << "] from command ["sv << cmd << ']';
+
+      // If the cmd path is not an absolute path, resolve it using our PATH variable
+      boost::filesystem::path cmd_path(parts.at(0));
+      if (!cmd_path.is_absolute()) {
+        cmd_path = boost::process::search_path(parts.at(0));
+        if (cmd_path.empty()) {
+          BOOST_LOG(error) << "Unable to find executable ["sv << parts.at(0) << "]. Is it in your PATH?"sv;
+          return boost::filesystem::path();
+        }
+      }
+
+      BOOST_LOG(debug) << "Resolved executable ["sv << parts.at(0) << "] to path ["sv << cmd_path << ']';
+
+      // Now that we have a complete path, we can just use parent_path()
+      return cmd_path.parent_path();
+    }


I'd suggest moving this to user_commands

Hazer · 2024-05-12T03:03:44Z

src/process.cpp

@@ -109,7 +109,7 @@ namespace proc {
  }

  boost::filesystem::path
-  find_working_directory(const std::string &cmd, bp::environment &env) {
+  find_working_directory(const std::string &cmd) {


Can this be merged with the other method with the same name?

Can this be moved to the same user_commands if something like this gets created?

Chris3773 added 2 commits February 12, 2024 20:12

Allow processes to be started on start/stop of a session.

4fc9b38

Fix variable assignment.

3c84318

Merge branch 'nightly' into on-session

1c31752

Chris3773 marked this pull request as draft February 13, 2024 03:01

Allow more env variables to be used for apps.

747f53a

Chris3773 marked this pull request as ready for review February 13, 2024 03:18

Chris3773 added 3 commits February 16, 2024 10:25

Merge branch 'nightly' into on-session

06d1dd9

Merge branch 'nightly' into on-session

e662f03

Merge branch 'nightly' into on-session

7b3f3c2

ReenigneArcher added this to the adjust lint rules milestone Feb 28, 2024

Hazer suggested changes May 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow processes to be started on start/stop of a session. #2138

Allow processes to be started on start/stop of a session. #2138

Chris3773 commented Feb 13, 2024 •

edited

CLAassistant commented Feb 13, 2024 •

edited

Hazer commented May 12, 2024

Hazer May 12, 2024

Hazer May 12, 2024

Hazer May 12, 2024

Hazer May 12, 2024

Hazer May 12, 2024

Hazer May 12, 2024

Hazer May 12, 2024

		#include <boost/process.hpp>

Allow processes to be started on start/stop of a session. #2138

Are you sure you want to change the base?

Allow processes to be started on start/stop of a session. #2138

Conversation

Chris3773 commented Feb 13, 2024 • edited

Description

Screenshot

Issues Fixed or Closed

Type of Change

Checklist

Branch Updates

CLAassistant commented Feb 13, 2024 • edited

Hazer commented May 12, 2024

Hazer May 12, 2024

Choose a reason for hiding this comment

Hazer May 12, 2024

Choose a reason for hiding this comment

Hazer May 12, 2024

Choose a reason for hiding this comment

Hazer May 12, 2024

Choose a reason for hiding this comment

Hazer May 12, 2024

Choose a reason for hiding this comment

Hazer May 12, 2024

Choose a reason for hiding this comment

Hazer May 12, 2024

Choose a reason for hiding this comment

Chris3773 commented Feb 13, 2024 •

edited

CLAassistant commented Feb 13, 2024 •

edited