Skip to content

[V2:04] Add Workspace Configuration Discovery#18

Open
ritorhymes wants to merge 1 commit into
eips-wg:masterfrom
ritovision:v2/04-workspace-config-discovery
Open

[V2:04] Add Workspace Configuration Discovery#18
ritorhymes wants to merge 1 commit into
eips-wg:masterfrom
ritovision:v2/04-workspace-config-discovery

Conversation

@ritorhymes

@ritorhymes ritorhymes commented May 5, 2026

Copy link
Copy Markdown
Contributor

Part of V2: Multi-repo local build system.

Summary

This PR adds the local workspace configuration layer that later init, doctor, runtime, and serve/preview PRs consume. It defines strict .build-eips.toml parsing, upward discovery, server/site defaults, and workspace path helpers; the [render].only surface is intentionally deferred to V2:15. LoadedWorkspaceConfig::discover is not called from main.rs yet, so review should focus on schema stability, discovery behavior, and derived path/default semantics rather than command integration.

  • Add .build-eips.toml workspace config parsing and upward discovery.
  • Add starter/default workspace config text.
  • Add server binding and site base URL settings.
  • Add workspace path helpers for build roots, the local theme, and local repos.
  • Add strict schema tests for defaults, invalid values, unsupported fields, and discovery behavior.

Review Notes

This is the workspace configuration foundation for the preprocessor stack. It defines how local workspaces are discovered and how workspace-local server/site/path defaults are represented, but it does not yet add user-facing workspace commands.

LoadedWorkspaceConfig::discover is defined here but not yet called from main.rs; init, doctor, and the runtime PRs wire it in.

Workspace init and doctor consume this in V2:06-V2:07. Execution policy consumes the loaded config in V2:08, and server binding is used by serve/preview in V2:11-V2:12.

[render].only is intentionally excluded here. Targeted rendering adds the render config surface in V2:15, when the build command can actually consume it.

The strict parse tests verify that unsupported workspace-config fields fail clearly instead of being silently ignored.

Review config.rs as a schema/discovery change: the important parts are strict parsing, stable starter output, upward discovery from active repo paths, and path helpers derived from the workspace root.

Verification

  • Review WorkspaceConfig, LoadedWorkspaceConfig, ServerSettings, ServerBinding, and SiteSettings in src/config.rs.
  • Review default_workspace_config_text() and the roundtrip/default tests for stable starter config output.
  • Review discover_path() and LoadedWorkspaceConfig::discover() for upward config discovery.
  • Review strict parse tests for unsupported fields and invalid site base URLs.

@ritorhymes ritorhymes force-pushed the v2/04-workspace-config-discovery branch from 208ad72 to 16168f9 Compare May 5, 2026 07:53
@ritorhymes ritorhymes changed the title Add Workspace Configuration Discovery [V2:04] Add Workspace Configuration Discovery May 5, 2026
@ritorhymes ritorhymes force-pushed the v2/04-workspace-config-discovery branch 2 times, most recently from 8d3c598 to e60a9b0 Compare May 9, 2026 20:10
@ritorhymes ritorhymes force-pushed the v2/04-workspace-config-discovery branch from e60a9b0 to 8c9b119 Compare May 26, 2026 01:02

@SamWilsn SamWilsn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hm, so is the intended directory structure something like:

  • ~/Workspace/.build-eips.toml
  • ~/Workspace/.local-build/...
  • ~/Workspace/ERCs/...
  • ~/Workspace/EIPs/...

?

Comment thread src/config.rs Outdated
Comment thread src/config.rs Outdated
Comment thread src/config.rs
}

impl WorkspaceConfig {
fn starter() -> Self {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems like a good candidate for a Default::default impl instead, unless there's a reason to not have a default?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There is a reason for keeping these separate. WorkspaceConfig already has Default: that is the omitted-user-config state, where site.base_url stays None so later code can distinguish “no override was configured” from “use this explicit base URL.” starter() is only for scaffolding a new .build-eips.toml, where it is useful to write an explicit local dev value like http://127.0.0.1:1111. Collapsing those would either make omitted config behave like an override, or make the generated starter file less useful.

Comment thread src/config.rs
Comment on lines +473 to +482
/// Workspace-local bind address defaults for local server commands.
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
#[serde(default, deny_unknown_fields)]
pub struct ServerSettings {
/// Host or interface address used by `serve` and `preview`.
pub host: String,

/// TCP port used by `serve` and `preview`.
pub port: u16,
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would a SocketAddr be more appropriate?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'd keep host and port separate. SocketAddr is an IP address plus a port, so it has nowhere to store a hostname. Making it accept localhost would mean resolving the name during config parsing, which turns reading a file into fallible name resolution and discards the original hostname. Keeping host: String stores what the user typed and lets the bind call resolve it when the server starts. Separate keys are also easier to read and edit in TOML.

@ritorhymes

Copy link
Copy Markdown
Contributor Author

Hm, so is the intended directory structure something like:

  • ~/Workspace/.build-eips.toml
  • ~/Workspace/.local-build/...
  • ~/Workspace/ERCs/...
  • ~/Workspace/EIPs/...

?

Exactly.

@ritorhymes

Copy link
Copy Markdown
Contributor Author

I need to rebase this to align with the repo manifest changes merged today and do some work adjusting the architecture downstream (which may cascade down to earlier branches, maybe here), especially regarding theme ownership.

Add workspace-local `.build-eips.toml` loading and starter configuration. Introduce `config::ActiveRepo` to load the selected checkout’s `Build.toml`, validate explicit `-C` roots, and expose active repository context to later commands. Keep source materialization, initialization, and diagnostics in their owning later branches.
@ritorhymes ritorhymes force-pushed the v2/04-workspace-config-discovery branch from 8c9b119 to 322e2f9 Compare June 21, 2026 22:41
@ritorhymes

Copy link
Copy Markdown
Contributor Author

Okay I just finished a system-wide reworking to accommodate the foundational changes introduced from #43's new architecture. This PR is ready to review and everything else is good downstream for this series.

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