Merge pull request #1619 from cgwalters/fix-man-sync

docs: Fix links to man pages

Author: cgwalters

Makefile

@@ -15,22 +15,9 @@ all: bin manpages
 bin:
 	cargo build --release --features "$(CARGO_FEATURES)"
 
-# Generate man pages from markdown sources
-MAN5_SOURCES := $(wildcard docs/src/man/*.5.md)
-MAN8_SOURCES := $(wildcard docs/src/man/*.8.md)
-TARGETMAN := target/man
-MAN5_TARGETS := $(patsubst docs/src/man/%.5.md,$(TARGETMAN)/%.5,$(MAN5_SOURCES))
-MAN8_TARGETS := $(patsubst docs/src/man/%.8.md,$(TARGETMAN)/%.8,$(MAN8_SOURCES))
-
-$(TARGETMAN)/%.5: docs/src/man/%.5.md
-	@mkdir -p $(TARGETMAN)
-	go-md2man -in $< -out $@
-
-$(TARGETMAN)/%.8: docs/src/man/%.8.md
-	@mkdir -p $(TARGETMAN)
-	go-md2man -in $< -out $@
-
-manpages: $(MAN5_TARGETS) $(MAN8_TARGETS)
+.PHONY: manpages
+manpages:
+	cargo run --package xtask -- manpages
 
 STORAGE_RELATIVE_PATH ?= $(shell realpath -m -s --relative-to="$(prefix)/lib/bootc/storage" /sysroot/ostree/bootc/storage)
 install:
@@ -41,12 +28,8 @@ install:
 	ln -s "$(STORAGE_RELATIVE_PATH)" "$(DESTDIR)$(prefix)/lib/bootc/storage"
 	install -D -m 0755 crates/cli/bootc-generator-stub $(DESTDIR)$(prefix)/lib/systemd/system-generators/bootc-systemd-generator 
 	install -d $(DESTDIR)$(prefix)/lib/bootc/install
-	if [ -n "$(MAN5_TARGETS)" ]; then \
-	  install -D -m 0644 -t $(DESTDIR)$(prefix)/share/man/man5 $(MAN5_TARGETS); \
-	fi
-	if [ -n "$(MAN8_TARGETS)" ]; then \
-	  install -D -m 0644 -t $(DESTDIR)$(prefix)/share/man/man8 $(MAN8_TARGETS); \
-	fi
+	install -D -m 0644 -t $(DESTDIR)$(prefix)/share/man/man5 target/man/*.5; \
+	install -D -m 0644 -t $(DESTDIR)$(prefix)/share/man/man8 target/man/*.8; \
 	install -D -m 0644 -t $(DESTDIR)/$(prefix)/lib/systemd/system systemd/*.service systemd/*.timer systemd/*.path systemd/*.target
 	install -d -m 0755 $(DESTDIR)/$(prefix)/lib/systemd/system/multi-user.target.wants
 	ln -s ../bootc-status-updated.path $(DESTDIR)/$(prefix)/lib/systemd/system/multi-user.target.wants/bootc-status-updated.path

crates/lib/src/cli_json.rs

@@ -13,6 +13,7 @@ pub struct CliOption {
     pub help: String,
     pub possible_values: Vec<String>,
     pub required: bool,
+    pub is_boolean: bool,
 }
 
 /// Representation of a CLI command for JSON export
@@ -73,16 +74,27 @@ pub fn command_to_json(cmd: &Command) -> CliCommand {
 
             let help = arg.get_help().map(|h| h.to_string()).unwrap_or_default();
 
+            // For boolean flags, don't show a value name
+            // Boolean flags use SetTrue or SetFalse actions and don't take values
+            let is_boolean = matches!(
+                arg.get_action(),
+                clap::ArgAction::SetTrue | clap::ArgAction::SetFalse
+            );
+            let value_name = if is_boolean {
+                None
+            } else {
+                arg.get_value_names()
+                    .and_then(|names| names.first())
+                    .map(|s| s.to_string())
+            };
+
             options.push(CliOption {
                 long: arg
                     .get_long()
                     .map(String::from)
                     .unwrap_or_else(|| id.to_string()),
                 short: arg.get_short().map(|c| c.to_string()),
-                value_name: arg
-                    .get_value_names()
-                    .and_then(|names| names.first())
-                    .map(|s| s.to_string()),
+                value_name,
                 default: arg
                     .get_default_values()
                     .first()
@@ -91,6 +103,7 @@ pub fn command_to_json(cmd: &Command) -> CliCommand {
                 help,
                 possible_values,
                 required: arg.is_required_set(),
+                is_boolean,
             });
         }
     }

crates/xtask/src/man.rs

@@ -26,6 +26,8 @@ pub struct CliOption {
     pub possible_values: Vec<String>,
     /// Whether the option is required
     pub required: bool,
+    /// Whether this is a boolean flag
+    pub is_boolean: bool,
 }
 
 /// Represents a CLI command from the JSON dump
@@ -132,16 +134,17 @@ fn format_options_as_markdown(options: &[CliOption], positionals: &[CliPositiona
         // Add long flag
         flag_line.push_str(&format!("**--{}**", opt.long));
 
-        // Add value name if option takes argument
+        // Add value name if option takes argument (but not for boolean flags)
+        // Boolean flags are detected by having no value_name (set to None in cli_json.rs)
         if let Some(value_name) = &opt.value_name {
             flag_line.push_str(&format!("=*{}*", value_name));
         }
 
         result.push_str(&format!("{}\n\n", flag_line));
         result.push_str(&format!("    {}\n\n", opt.help));
 
-        // Add possible values for enums
-        if !opt.possible_values.is_empty() {
+        // Add possible values for enums (but not for boolean flags)
+        if !opt.possible_values.is_empty() && !opt.is_boolean {
             result.push_str("    Possible values:\n");
             for value in &opt.possible_values {
                 result.push_str(&format!("    - {}\n", value));
@@ -191,10 +194,12 @@ pub fn update_markdown_with_subcommands(
         before, begin_marker, generated_subcommands, end_marker, after
     );
 
-    fs::write(markdown_path, new_content)
-        .with_context(|| format!("Writing to {}", markdown_path))?;
-
-    println!("Updated subcommands in {}", markdown_path);
+    // Only write if content has changed to avoid updating mtime unnecessarily
+    if new_content != content {
+        fs::write(markdown_path, new_content)
+            .with_context(|| format!("Writing to {}", markdown_path))?;
+        println!("Updated subcommands in {}", markdown_path);
+    }
     Ok(())
 }
 
@@ -238,10 +243,12 @@ pub fn update_markdown_with_options(
         format!("{}\n\n{}\n{}{}", before, begin_marker, end_marker, after)
     };
 
-    fs::write(markdown_path, new_content)
-        .with_context(|| format!("Writing to {}", markdown_path))?;
-
-    println!("Updated {}", markdown_path);
+    // Only write if content has changed to avoid updating mtime unnecessarily
+    if new_content != content {
+        fs::write(markdown_path, new_content)
+            .with_context(|| format!("Writing to {}", markdown_path))?;
+        println!("Updated {}", markdown_path);
+    }
     Ok(())
 }
 
@@ -374,21 +381,6 @@ pub fn sync_all_man_pages(sh: &Shell) -> Result<()> {
     Ok(())
 }
 
-/// Test the sync workflow
-pub fn test_sync_workflow(sh: &Shell) -> Result<()> {
-    println!("🧪 Testing man page sync workflow...");
-
-    // Create a backup of current files
-    let test_dir = "target/test-sync";
-    sh.create_dir(test_dir)?;
-
-    // Run sync
-    sync_all_man_pages(sh)?;
-
-    println!("✅ Sync workflow test completed successfully");
-    Ok(())
-}
-
 /// Generate man pages from hand-written markdown sources
 pub fn generate_man_pages(sh: &Shell) -> Result<()> {
     let man_src_dir = Utf8Path::new("docs/src/man");
@@ -432,18 +424,31 @@ pub fn generate_man_pages(sh: &Shell) -> Result<()> {
         let content = fs::read_to_string(&path)?;
         let content_with_version = content.replace("<!-- VERSION PLACEHOLDER -->", &version);
 
-        // Create temporary file with version-replaced content
-        let temp_path = format!("{}.tmp", path.display());
-        fs::write(&temp_path, content_with_version)?;
+        // Check if we need to regenerate by comparing input and output modification times
+        let should_regenerate = if let (Ok(input_meta), Ok(output_meta)) =
+            (fs::metadata(&path), fs::metadata(&output_file))
+        {
+            input_meta.modified().unwrap_or(std::time::UNIX_EPOCH)
+                > output_meta.modified().unwrap_or(std::time::UNIX_EPOCH)
+        } else {
+            // If output doesn't exist or we can't get metadata, regenerate
+            true
+        };
+
+        if should_regenerate {
+            // Create temporary file with version-replaced content
+            let temp_path = format!("{}.tmp", path.display());
+            fs::write(&temp_path, content_with_version)?;
 
-        cmd!(sh, "go-md2man -in {temp_path} -out {output_file}")
-            .run()
-            .with_context(|| format!("Converting {} to man page", path.display()))?;
+            cmd!(sh, "go-md2man -in {temp_path} -out {output_file}")
+                .run()
+                .with_context(|| format!("Converting {} to man page", path.display()))?;
 
-        // Clean up temporary file
-        fs::remove_file(&temp_path)?;
+            // Clean up temporary file
+            fs::remove_file(&temp_path)?;
 
-        println!("Generated {}", output_file);
+            println!("Generated {}", output_file);
+        }
     }
 
     // Apply post-processing fixes for apostrophe handling
@@ -495,9 +500,9 @@ pub fn update_manpages(sh: &Shell) -> Result<()> {
     // Check each command and create man page if missing
     for command_parts in commands_to_check {
         let filename = if command_parts.len() == 1 {
-            format!("bootc-{}.md", command_parts[0])
+            format!("bootc-{}.8.md", command_parts[0])
         } else {
-            format!("bootc-{}.md", command_parts.join("-"))
+            format!("bootc-{}.8.md", command_parts.join("-"))
         };
 
         let filepath = format!("docs/src/man/{}", filename);
@@ -511,14 +516,31 @@ pub fn update_manpages(sh: &Shell) -> Result<()> {
                 let command_name_full = format!("bootc {}", command_parts.join(" "));
                 let command_description = cmd.about.as_deref().unwrap_or("TODO: Add description");
 
+                // Generate SYNOPSIS line with proper arguments
+                let mut synopsis = format!("**{}** \\[*OPTIONS...*\\]", command_name_full);
+
+                // Add positional arguments
+                for positional in &cmd.positionals {
+                    if positional.required {
+                        synopsis.push_str(&format!(" <*{}*>", positional.name.to_uppercase()));
+                    } else {
+                        synopsis.push_str(&format!(" \\[*{}*\\]", positional.name.to_uppercase()));
+                    }
+                }
+
+                // Add subcommand if this command has subcommands
+                if !cmd.subcommands.is_empty() {
+                    synopsis.push_str(" <*SUBCOMMAND*>");
+                }
+
                 let template = format!(
                     r#"# NAME
 
 {} - {}
 
 # SYNOPSIS
 
-**{}** [*OPTIONS*]
+{}
 
 # DESCRIPTION
 
@@ -549,19 +571,19 @@ TODO: Add practical examples showing how to use this command.
                 std::fs::write(&filepath, template)
                     .with_context(|| format!("Writing template to {}", filepath))?;
 
-                println!("✓ Created man page template: {}", filepath);
+                println!("Created man page template: {}", filepath);
                 created_count += 1;
             }
         }
     }
 
     if created_count > 0 {
-        println!("✓ Created {} new man page templates", created_count);
+        println!("Created {} new man page templates", created_count);
     } else {
-        println!("✓ All commands already have man pages");
+        println!("All commands already have man pages");
     }
 
-    println!("🔄 Syncing OPTIONS sections...");
+    println!("Syncing OPTIONS sections...");
     sync_all_man_pages(sh)?;
 
     println!("Man pages updated.");
@@ -585,6 +607,13 @@ fn apply_man_page_fixes(sh: &Shell, dir: &Utf8Path) -> Result<()> {
             .and_then(|s| s.to_str())
             .map_or(false, |e| e.chars().all(|c| c.is_numeric()))
         {
+            // Check if the file already has the fix applied
+            let content = fs::read_to_string(&path)?;
+            if content.starts_with(".ds Aq \\(aq\n") {
+                // Already fixed, skip
+                continue;
+            }
+
             // Apply the same sed fixes as before
             let groffsub = r"1i .ds Aq \\(aq";
             let dropif = r"/\.g \.ds Aq/d";

crates/xtask/src/xtask.rs

@@ -36,9 +36,6 @@ fn main() {
 #[allow(clippy::type_complexity)]
 const TASKS: &[(&str, fn(&Shell) -> Result<()>)] = &[
     ("manpages", man::generate_man_pages),
-    ("sync-manpages", man::sync_all_man_pages),
-    ("test-sync-manpages", man::test_sync_workflow),
-    ("update-json-schemas", update_json_schemas),
     ("update-generated", update_generated),
     ("package", package),
     ("package-srpm", package_srpm),
@@ -47,17 +44,21 @@ const TASKS: &[(&str, fn(&Shell) -> Result<()>)] = &[
 ];
 
 fn try_main() -> Result<()> {
-    // Ensure our working directory is the toplevel
+    // Ensure our working directory is the toplevel (if we're in a git repo)
     {
-        let toplevel_path = Command::new("git")
+        if let Ok(toplevel_path) = Command::new("git")
             .args(["rev-parse", "--show-toplevel"])
             .output()
-            .context("Invoking git rev-parse")?;
-        if !toplevel_path.status.success() {
-            anyhow::bail!("Failed to invoke git rev-parse");
+        {
+            if toplevel_path.status.success() {
+                let path = String::from_utf8(toplevel_path.stdout)?;
+                std::env::set_current_dir(path.trim()).context("Changing to toplevel")?;
+            }
+        }
+        // Otherwise verify we're in the toplevel
+        if !Utf8Path::new("ADOPTERS.md").try_exists()? {
+            anyhow::bail!("Not in toplevel")
         }
-        let path = String::from_utf8(toplevel_path.stdout)?;
-        std::env::set_current_dir(path.trim()).context("Changing to toplevel")?;
     }
 
     let task = std::env::args().nth(1);
@@ -393,11 +394,20 @@ fn update_json_schemas(sh: &Shell) -> Result<()> {
     Ok(())
 }
 
-/// Update generated files using the new sync approach
+/// Update all generated files
+/// This is the main command developers should use to update generated content.
+/// It handles:
+/// - Creating new man page templates for new commands
+/// - Syncing CLI options to existing man pages
+/// - Updating JSON schema files
 #[context("Updating generated files")]
 fn update_generated(sh: &Shell) -> Result<()> {
+    // Update man pages (create new templates + sync options)
     man::update_manpages(sh)?;
+
+    // Update JSON schemas
     update_json_schemas(sh)?;
+
     Ok(())
 }