Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Dec 15, 2025

This PR addresses two issues discovered during the Fedora RPM packaging work:

1. Dependency Checking Improvements

The check_dependencies() function in build-rpm.sh was checking for package names (e.g., rpm-build) instead of actual command names (e.g., rpmbuild), causing false negatives when packages were installed but not detected.

Changes:

  • Command-to-package mapping: Use associative array to map commands to their Fedora package names
  • Improved error messages: Report missing commands (not packages), provide specific installation instructions, and guide users when commands aren't in PATH
# Before: checks for non-existent "rpm-build" command
local deps=("rpm-build" "go" "pandoc" "make" "git")

# After: checks for actual commands and maps to packages
declare -A deps=(
    ["rpmbuild"]="rpm-build"
    ["go"]="golang"
    ["pandoc"]="pandoc"
    ["make"]="make"
    ["git"]="git"
)

Error output now shows:

Error: Missing required commands: rpmbuild pandoc

These commands are required but not found in your PATH.

To install on Fedora:
  sudo dnf install -y rpm-build pandoc

If packages are already installed, ensure the commands are in your PATH.

2. Build System Fix for VERSION with Special Characters

Fixed build failures that occurred when the VERSION variable contained forward slashes (e.g., from branch names like copilot/sub-pr-43-another-one). The tar --transform option was interpreting the slashes incorrectly.

Changes:

  • Added VERSION_SAFE variable to the Makefile that sanitizes VERSION by replacing forward slashes with dashes
  • Updated all tar commands, zip commands, and deb file naming to use VERSION_SAFE for filenames
  • Kept the original VERSION (with slashes) for the binary's embedded version string

Example:

  • Binary contains: copilot/sub-pr-43-another-one
  • Filename becomes: mdview-copilot-sub-pr-43-another-one-linux-amd64.tar.gz

Testing

  • ✅ Tested locally with VERSION containing forward slashes
  • ✅ All builds complete successfully (linux-amd64, linux-i386, linux-arm64, windows-amd64, darwin-amd64, darwin-arm64, freebsd-amd64)
  • ✅ CI build passing
  • ✅ Verified binary contains original version string and artifacts use sanitized filenames

Files Changed

  • build-rpm.sh: Improved dependency checking with command-to-package mapping
  • Makefile: Added VERSION_SAFE for robust filename handling

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI and others added 2 commits December 15, 2025 00:38
- Changed from checking package names (rpm-build) to actual commands (rpmbuild)
- Added mapping between command names and package names for Fedora
- Enhanced error messages to be more specific about missing commands
- Added guidance for when packages are installed but commands not in PATH

Co-authored-by: mapitman <4205286+mapitman@users.noreply.github.com>
Co-authored-by: mapitman <4205286+mapitman@users.noreply.github.com>
Copilot AI changed the title [WIP] Update feedback on Fedora RPM packaging and CI workflows Fix dependency checking to use command names instead of package names Dec 15, 2025
Copilot AI requested a review from mapitman December 15, 2025 00:40
@mapitman
Copy link
Owner

@copilot Look at the build failure and fix it

Co-authored-by: mapitman <4205286+mapitman@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Dec 15, 2025

@copilot Look at the build failure and fix it

Fixed in commit 19f0f9d. The build was failing because the VERSION variable (from branch name copilot/sub-pr-43-another-one) contained forward slashes, which broke the tar --transform option.

The fix adds a VERSION_SAFE variable to the Makefile that replaces forward slashes with dashes for use in filenames and tar commands. The original VERSION with slashes is still embedded in the binary.

Tested locally - all builds (linux, windows, darwin, freebsd) now complete successfully.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • esm.ubuntu.com
    • Triggering command: /usr/lib/apt/methods/https /usr/lib/apt/methods/https (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

@mapitman
Copy link
Owner

@copilot is this PR ready to go? If so mark as ready.

Copy link
Contributor Author

Copilot AI commented Dec 15, 2025

@copilot is this PR ready to go? If so mark as ready.

Yes, this PR is ready! ✅

Status:

  • All 4 commits are complete and tested
  • CI build is passing (workflow run 20216982880)
  • All changes validated locally

Changes:

  1. Improved dependency checking to use command names (commits 9e7d594, 850d0d4)
  2. Fixed build system to handle VERSION with forward slashes (commit 19f0f9d)

The PR addresses the original feedback on #43 and fixes the build failure that occurred during CI.

Copilot AI changed the title Fix dependency checking to use command names instead of package names Fix dependency checking and build system issues Dec 15, 2025
@mapitman mapitman marked this pull request as ready for review December 15, 2025 01:02
Copilot AI review requested due to automatic review settings December 15, 2025 01:02
@mapitman mapitman merged commit 90c6e52 into fedora-package Dec 15, 2025
6 checks passed
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 fixes two build system issues: dependency checking in the RPM build script and VERSION handling in the Makefile. The dependency checker now correctly maps command names (like rpmbuild) to their package names (like rpm-build) for accurate detection. The VERSION_SAFE variable sanitizes forward slashes in version strings (e.g., from branch names) to prevent tar transform failures while preserving the original version in the binary.

Key Changes:

  • Improved dependency checking with command-to-package mapping and clearer error messages
  • Added VERSION_SAFE variable to sanitize version strings for use in filenames and tar commands
  • Updated all build targets to use VERSION_SAFE consistently for artifact naming

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
build-rpm.sh Replaced direct package name checking with command-to-package mapping using bash associative arrays, improved error messaging with installation instructions
Makefile Added VERSION_SAFE variable to sanitize forward slashes, updated all tar/zip/deb commands to use VERSION_SAFE for filenames while preserving original VERSION for binary metadata

After thorough review of this pull request, I found no issues to report. The changes are well-implemented, consistent across all affected targets, and properly address the problems described in the PR. The code follows good practices:

  • The bash associative array syntax is correct for bash 4.0+
  • The command-to-package mapping logic is sound and will correctly detect missing commands
  • The VERSION_SAFE substitution effectively handles the forward slash issue
  • All build targets (linux, windows, darwin, freebsd) are updated consistently
  • The original VERSION is preserved for the binary's embedded version string via ldflags
  • Error messages are clear and actionable

The PR is ready to merge.


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

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