Add local workspace and site build/check support - #10
Conversation
Add the local workspace model for the multi-repo build system. This adds workspace config and profile support, workspace init, refresh, and doctor commands, generated helper tasks, and the local site build path against sibling repos and theme inputs. The site commands support normal local usage, parity checks, standard clone and git worktree checkouts, and explicit dirty builds from the active content repo.
SamWilsn
left a comment
SamWilsn
left a comment
There was a problem hiding this comment.
I've only reviewed config.rs, find_root.rs, and main.rs so far, and only for minutia (no architecture or even "is this the right path?" yet).
Each pull request should be one feature, or else they're way too big for me to thoroughly review. This pull request adds:
- workspaces
- system command detection
- support for local/remote themes+other repo
- dirty git working trees
I hate to give you even more work, because I really appreciate what you've done so far, but there's no way I can give your contributions the attention they deserve when they're this dense.
Random thought, but would something like config be useful for organizing the different ways of specifying configuration options?
| pub default_profile: Option<String>, | ||
| pub build_root_base: PathBuf, | ||
| pub profiles: HashMap<String, LocalProfile>, |
There was a problem hiding this comment.
I'm not normally a stickler for doc comments, but because these (presumably) end up in a user facing config file, I think they deserve some.
There was a problem hiding this comment.
Okay, I'll add that. It's not currently in the generated config file today, since that starter text is still hand-written, but I agree it is still a user-facing schema and worth documenting.
| impl Default for WorkspaceConfig { | ||
| fn default() -> Self { | ||
| Self { | ||
| default_profile: Some(DEFAULT_PROFILE.into()), | ||
| build_root_base: DEFAULT_BUILD_ROOT_BASE.into(), | ||
| profiles: HashMap::new(), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Just for my sanity, lets keep the impls near the structs:
| impl Default for WorkspaceConfig { | |
| fn default() -> Self { | |
| Self { | |
| default_profile: Some(DEFAULT_PROFILE.into()), | |
| build_root_base: DEFAULT_BUILD_ROOT_BASE.into(), | |
| profiles: HashMap::new(), | |
| } | |
| } | |
| } |
| pub default_profile: Option<String>, | ||
| pub build_root_base: PathBuf, | ||
| pub profiles: HashMap<String, LocalProfile>, | ||
| } |
There was a problem hiding this comment.
| } | |
| } | |
| impl Default for WorkspaceConfig { | |
| fn default() -> Self { | |
| Self { | |
| default_profile: Some(DEFAULT_PROFILE.into()), | |
| build_root_base: DEFAULT_BUILD_ROOT_BASE.into(), | |
| profiles: HashMap::new(), | |
| } | |
| } | |
| } |
|
|
||
| pub fn from_path(path: &Path) -> Result<Self, WorkspaceError> { | ||
| let path = path.canonicalize().context(FsSnafu { | ||
| path: path.to_path_buf(), |
There was a problem hiding this comment.
Is to_path_buf required here? I believe snafu uses Into, but I could be wrong.
There was a problem hiding this comment.
I just verified, you're right, it's not required, I'll clean it up.
| let curl = command_path("curl"); | ||
| let wget = command_path("wget"); |
There was a problem hiding this comment.
Just use reqwest (or reqwest::blocking) or ureq. We don't need to shell out to these.
There was a problem hiding this comment.
I agree with the direction if the install/update path shifts so Rust owns downloads rather than scripts/dev-setup handling first-install in shell. That shift naturally raises additional decisions around release distribution, integrity/verification, and retry/TLS handling, and a crate like reqwest or ureq is the right fit once a Rust-side consumer exists. I'm open to exploring that if you want to move in that direction now, but it would be a significant scope expansion for this reroll.
| ), | ||
| (None, None) => report.record( | ||
| DoctorStatus::Warn, | ||
| "missing both `curl` and `wget`; `scripts/dev-setup` will not be able to download a release binary", |
There was a problem hiding this comment.
Ah. Should this have been included in a later pull request?
There was a problem hiding this comment.
Yeah, looks like it. I’ll clean that up.
| ); | ||
| check_optional_download_tool(&mut report); | ||
|
|
||
| match command_path("tar") { |
There was a problem hiding this comment.
If this is indeed necessary, tar is a good crate.
There was a problem hiding this comment.
I agree with the direction if the install/update path shifts so Rust owns download and unpack rather than scripts/dev-setup handling first-install in shell. That shift brings its own decisions around release distribution, integrity/verification, and self-replacing-binary semantics on Windows, and the tar crate becomes the right fit once a Rust-side consumer exists. I'm open to exploring that if you want to move in that direction now, but it would be a significant scope expansion for this reroll.
| .transpose()?, | ||
| }; | ||
|
|
||
| let use_staging = args.staging |
There was a problem hiding this comment.
Should there be an equivalent --no-staging flag?
There was a problem hiding this comment.
Yes, I think the right answer is to make these explicit CLI overrides rather than one-way additive flags. In the reroll I’m planning to treat staging (and similarly allow_dirty) as a tri-state override: --staging forces on, --no-staging forces off, and if neither is passed, behavior defers to the selected profile or built-in default.
| let args = Args::parse(); | ||
| if let Operation::Print { print } = args.operation { | ||
| print::print(print); | ||
| fn clone_missing_repo(url: &str, destination: &Path) -> Result<(), Whatever> { |
There was a problem hiding this comment.
This feels like it should be in the git module
|
@SamWilsn thanks for your review so far. I am working on a new reroll as we speak based on your specific feedback. I'm going to try and split the rerolled version of this PR into separate PR's based on the features you outlined, TBD how clean or messy that may look since we're splitting a system up. I expect to have it ready within the next couple of days. I'll be dropping additional comments and replies to your review in this thread in the meantime. |
|
I care about making this idiomatic and maintainable. Rust isn’t my primary language (TypeScript is my home), so please do keep flagging those conventions and preferences when you see them. Where there are more idiomatic or efficient patterns I should be using, or other changes that would make the code more legible to you and others, I’m very open to that guidance and I’ll make a point of incorporating it. |
I looked into My reasoning:
So my current leaning is to skip |
Yes, that is a core part of it. The intent is to make running from inside
I agree, that is a wayyyyy better approach. I'm planning to drop justfile and move that shortcut surface into
A mode is how the build behaves (staging, dirty, parity, etc.); a profile is a named instance of a mode, a bundle of settings in
Allow me to give you a semi-quick breakdown of the vision and direction of this system's dev experience:
Anti-pattern direction That may be technically correct, but it is not a good contributor-facing interface. My goal here is to keep Influence from Jekyll This system is naturally more complex than that, so I am not trying to force it into a single generic config model. The local profiles in The goal is still the same: avoid making contributors type or reconstruct commands like: Rust, Zola, and the multi-repo model do raise the bar relative to something like Jekyll. I think that makes it more important that we offset the added complexity by reducing friction with a more streamlined command surface. Ergonomic CLI direction from modern frameworks I think that the CLI structure of modern frameworks like nextjs/astro/vue etc is wonderfully simple and a directional aspiration (mind you that complex monorepos may have their own elaborate and verbose CLI direction, but that is a different case entirely). With NextJS, you can go from 0 to a running dev server in two commands with four words: Dev server is running!
|
|
That makes a lot of sense. I do value user experience quite a bit but I also have to balance that against maintenance burden. Is there a single workspace layout we can just mandate and make it unconfigurable? Ideally I'd like the CLI to be as simple as Zola's, eventually. Maybe |
Yeah, we can pare back support for the out-of-tree style configuration options. No one will die.
If you don't think more flexibility buys much, I'm not inclined to push back here. My thesis is mainly around the baseline devEx being reasonably usable locally. While I can imagine various use cases being useful based on practical contexts, we don't have a proven demand for all of them or a clear critical need that would justify more complex maintenance. So if you don't see the benefit and no one is screaming for it (that I'm aware of), I'll see what I can reasonably scale back. I could be wrong too here, but we could always expand the product surface later if people do scream about it.
I'm going to take another pass (I was almost finished with the v2 reroll) to rework it closer to the above, though I'd expect there will still be flags / configs in some cases. |
2 participants
Important
This is the first PR in a stacked
preprocessorseries for preprocessor#8. That series splits the original implementation from preprocessor#9 into smaller reviewable PRs. This first slice introduces the local workspace and site build/check foundation. Later stacked PRs add the explicit editorial command surface, local serving and preview, and the public workflow documentation.Description
This PR adds the first usable foundation for the local multi-repo build system in
preprocessor.It introduces the local workspace model, the workspace config/profile layer, and the site build/check path against sibling repo and theme inputs. The goal of this slice is to make the system locally usable without pulling in the later editorial and serving features yet.
Changes
workspace init,workspace refresh, andworkspace doctorjustfilefor common taskscheckandbuildpathsNot Yet In This PR
This PR does not yet add:
serve/previewThose land in the later stacked PRs.
Validation
This foundation slice builds cleanly on its own:
cargo buildThe broader runtime paths for the local build system were validated on the original combined implementation in preprocessor#9 before restacking, including clean and dirty site builds, parity-oriented checks, and downstream staging verification from the rewritten EIPs#10 and ERCs#10 branches.
This stacked series preserves the final tracked tree of the original
local-build-systemimplementation from preprocessor#9 exactly.