Skip to content

runtime config clean#258

Open
cboulay wants to merge 3 commits intodevfrom
feature/runtime-config-clean
Open

runtime config clean#258
cboulay wants to merge 3 commits intodevfrom
feature/runtime-config-clean

Conversation

@cboulay
Copy link
Collaborator

@cboulay cboulay commented Jan 19, 2026

Clean rebase of #245 by @zeyus

Original PR: #245

Which was itself a redo of #234 but from a dedicated branch and targeting cboulay/apple_framework

On non-desktop platforms, often apps are sandboxed and don't have (read/write) access at all to $HOME, /etc/ or even the current working directory. These paths may not exist.

In addition, environment variables don't necessarily make a lot of sense in the context of a mobile app, and the application data paths might contain GUIDs or other aspects of the path that make it difficult to determine at compile-time.

This PR adds the ability to set a configuration file path for liblsl provided that the static member function lsl_set_config_filename is called before any other LSL function.

In addition, there's also an option to read config directly from a string stream via set_api_config_content which is loaded into the INI and discarded.

@cboulay cboulay mentioned this pull request Jan 19, 2026
@cboulay cboulay requested a review from Copilot January 19, 2026 17:32
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces runtime configuration capabilities for liblsl to support sandboxed environments where traditional file paths may be inaccessible. It enables applications to specify configuration either via direct string content or custom file paths before LSL initialization.

Changes:

  • Added two new C API functions (lsl_set_config_filename and lsl_set_config_content) for runtime configuration
  • Refactored configuration loading logic to support multiple configuration sources with defined precedence
  • Updated documentation to reflect the new configuration priority order

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
src/common.cpp Implements the new C API wrapper functions for setting config filename and content
src/api_config.h Declares new static methods and member variables for runtime config, updates documentation
src/api_config.cpp Implements configuration loading from content/custom files with proper precedence handling
include/lsl/common.h Adds public C API declarations for the new configuration functions
Comments suppressed due to low confidence (2)

include/lsl/common.h:1

  • The documentation for lsl_set_config_filename contains a sentence fragment. The sentence starting with 'If, and only if...' should be integrated into the previous sentence or rewritten as a complete sentence. Consider: 'This setting only takes effect if this function is called before any other LSL function.'
#pragma once

include/lsl/common.h:1

  • The documentation for lsl_set_config_content contains the same sentence fragment as lsl_set_config_filename. The sentence starting with 'If, and only if...' should be integrated into the previous sentence or rewritten as a complete sentence. Consider: 'This setting only takes effect if this function is called before any other LSL function.'
#pragma once

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +90 to +92
static void set_api_config_content(const std::string &content) {
api_config_content_ = content;
}
Copy link

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent indentation: this line uses spaces instead of tabs like the surrounding code. The function should be indented with a single tab to match the style of other methods in this class.

Suggested change
static void set_api_config_content(const std::string &content) {
api_config_content_ = content;
}
static void set_api_config_content(const std::string &content) {
api_config_content_ = content;
}

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't commit to this PR, but here's the diff:

diff --git a/src/api_config.h b/src/api_config.h
index 6cb987cd..d1b85c4a 100644
--- a/src/api_config.h
+++ b/src/api_config.h
@@ -79,17 +79,15 @@ public:
        bool allow_ipv6() const { return allow_ipv6_; }
        bool allow_ipv4() const { return allow_ipv4_; }
 
-
-
        /**
        * @brief Set the configuration directly from a string.
        * 
        * This allows passing in configuration content directly rather than from a file.
        * This MUST be called before the first call to get_instance() to have any effect.
        */
-    static void set_api_config_content(const std::string &content) {
-        api_config_content_ = content;
-    }
+       static void set_api_config_content(const std::string &content) {
+               api_config_content_ = content;
+       }
 
        /**
         * @brief An additional settings path to load configuration from.
@@ -105,7 +103,6 @@ public:
                api_config_filename_ = filename;
        }
 
-
        /**
         * @brief The range or scope of stream lookup when using multicast-based discovery
         *

@cboulay
Copy link
Collaborator Author

cboulay commented Jan 20, 2026

Comparison with PR #152

There's an older PR #152 by @tstenner that also attempted to make the config runtime-configurable. Here's how the two approaches differ:

PR #152 (tstenner) - Architectural Refactor

  • Refactors the entire config system with cleaner architecture
  • Callback-based INI parser instead of map-based
  • Adds set_option(section, key, value) method for per-option configuration
  • Default values moved to member initializers
  • Constructor takes bool skip_load_settings for testing
  • No new C API functions yet - internal only

PR #245 (zeyus) - Minimal Feature Addition

  • Minimal changes - adds two C API functions
  • Static members store config before singleton initialization
  • Targeted at sandboxed platforms (iOS/Android) that can't use env vars
  // New C API functions
  extern LIBLSL_C_API void lsl_set_config_filename(const char *filename);
  extern LIBLSL_C_API void lsl_set_config_content(const char *content);

Summary

Aspect PR #152 PR #245
Scope Large refactor Minimal addition
C API Not exposed yet Two new functions
Flexibility Set individual options Set file path or full content
Use case General runtime config Sandboxed platforms
Architecture Cleaner, callback-based Simple static strings
Merge difficulty High (stale, touches many areas) Low (small diff, but branch has conflicts)

PR #245's approach is simpler and addresses the immediate need for sandboxed platforms. PR #152's set_option() could be added later as an enhancement.

I'm planning to cherry-pick the relevant commits from this PR onto a clean branch to resolve the merge conflicts. Credit will be preserved via git history.

@zeyus
Copy link
Contributor

zeyus commented Jan 27, 2026

By the way, regarding PR #152 I think it would be valuable to be able to re-configure via the API at any point.

Arguably (IMHO) this would require quite a bit of additional work because it allows setting arbitrary values, including listen_address, which would be great, but wouldn't actually do anything once the UDP server has been created. This could also cause future outlets to become mismatched with the UDP bind address(es).

I probably wouldn't mind taking a look at this at some point, but if it hasn't yet been addressed, I think the crash when interfaces change might need to be fixed first. It seems feasible, and I just had a cursory look and came across a bug that might contribute to the issue. I'll make a PR for it.

@zeyus
Copy link
Contributor

zeyus commented Jan 27, 2026

@cboulay see #259

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.

3 participants