Skip to content

Further improvements to macos installation tasks#45

Merged
volpatto merged 2 commits intomainfrom
fb-fix-installation-tasks-macos-further
Feb 26, 2026
Merged

Further improvements to macos installation tasks#45
volpatto merged 2 commits intomainfrom
fb-fix-installation-tasks-macos-further

Conversation

@volpatto
Copy link
Member

@volpatto volpatto commented Feb 26, 2026

Fix a Bison frozen installation issue.

@volpatto volpatto added the native job Run CI build installing Firedrake from scratch label Feb 26, 2026
@github-actions
Copy link

Coverage report

This PR does not seem to contain any modification to coverable code.

Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
@volpatto volpatto marked this pull request as ready for review February 26, 2026 13:22
Copilot AI review requested due to automatic review settings February 26, 2026 13:22
@volpatto volpatto enabled auto-merge (squash) February 26, 2026 13:22
@volpatto volpatto merged commit be255dd into main Feb 26, 2026
9 checks passed
@volpatto volpatto deleted the fb-fix-installation-tasks-macos-further branch February 26, 2026 13:24
Copy link
Contributor

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 addresses a Bison frozen installation issue on macOS by preferring system-installed GNU Bison over PETSc's source build path, which can stall on newer macOS releases. The changes ensure that a modern Bison (≥3.0) is installed via Homebrew and detected before PETSc configuration, preventing long-running or hanging configure processes.

Changes:

  • Added automatic installation of bison package on macOS via Homebrew
  • Implemented detection and validation of system Bison executables with version checking
  • Added graceful fallback for Fortran compiler absence on macOS when ScaLAPACK is requested

Comment on lines +232 to +233
first_line = (probe.stdout or "").splitlines()
version_line = first_line[0] if first_line else ""
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

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

The variable name first_line is misleading because it contains a list of all lines from splitlines(), not just the first line. Consider renaming it to lines for clarity, as in: lines = (probe.stdout or "").splitlines() and version_line = lines[0] if lines else "".

Suggested change
first_line = (probe.stdout or "").splitlines()
version_line = first_line[0] if first_line else ""
lines = (probe.stdout or "").splitlines()
version_line = lines[0] if lines else ""

Copilot uses AI. Check for mistakes.
cfg_flags = [
f
for f in cfg_flags
if not any(keyword in f for keyword in ["scalapack", "fortran-bindings"])
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

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

The keyword filtering uses substring matching (any(keyword in f for keyword in ["scalapack", "fortran-bindings"])), which is less precise than the prefix-based matching used earlier in the code (line 680: f.startswith("--with-fortran-bindings=")). While this may work for current PETSc flags, it could inadvertently filter out unrelated flags containing these substrings. Consider using more specific filtering with startswith checks for known flag patterns, similar to the approach used in lines 652-660 for bison flags.

Suggested change
if not any(keyword in f for keyword in ["scalapack", "fortran-bindings"])
if not (
f.startswith("--with-scalapack")
or f.startswith("--download-scalapack")
or f.startswith("--with-fortran-bindings=")
)

Copilot uses AI. Check for mistakes.
all_pkgs = base_pkgs + ["gcc"]
# Also install modern bison (keg-only via Homebrew) so PETSc does not have
# to build bison from source, which can stall on newer macOS releases.
all_pkgs = list(dict.fromkeys(base_pkgs + ["gcc", "bison"]))
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

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

The deduplication approach using dict.fromkeys() differs from the explicit set-based deduplication pattern used consistently elsewhere in this file (see lines 157-162, 216-221, 726-731). For consistency with the established codebase convention, consider using the same pattern:

seen = set()
all_pkgs = []
for pkg in base_pkgs + ["gcc", "bison"]:
    if pkg not in seen:
        all_pkgs.append(pkg)
        seen.add(pkg)

While dict.fromkeys() is functionally correct, using the consistent pattern improves code readability and maintainability.

Suggested change
all_pkgs = list(dict.fromkeys(base_pkgs + ["gcc", "bison"]))
seen = set()
all_pkgs = []
for pkg in base_pkgs + ["gcc", "bison"]:
if pkg not in seen:
all_pkgs.append(pkg)
seen.add(pkg)

Copilot uses AI. Check for mistakes.

first_line = (probe.stdout or "").splitlines()
version_line = first_line[0] if first_line else ""
match = re.search(r"GNU Bison\)?\s+(\d+)\.(\d+)", version_line)
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

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

The regex pattern for parsing GNU Bison version has an awkward structure that may not handle all output formats robustly. The pattern r"GNU Bison\)?\s+(\d+)\.(\d+)" expects an optional closing parenthesis directly after "GNU Bison", which works for "bison (GNU Bison) 3.8.2" by matching "GNU Bison) 3.8", but could fail for variations in formatting. A more robust pattern would be r"[Bb]ison.*?(\d+)\.(\d+)" which matches any line containing "bison" or "Bison" followed by a version number, handling variations like "GNU Bison 3.0", "bison (GNU Bison) 3.8.2", and potential future format changes.

Suggested change
match = re.search(r"GNU Bison\)?\s+(\d+)\.(\d+)", version_line)
match = re.search(r"[Bb]ison.*?(\d+)\.(\d+)", version_line)

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

native job Run CI build installing Firedrake from scratch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants