Skip to content

Refactor firedrake configure#5073

Open
connorjward wants to merge 12 commits intoreleasefrom
connorjward/refactor-firedrake-configure
Open

Refactor firedrake configure#5073
connorjward wants to merge 12 commits intoreleasefrom
connorjward/refactor-firedrake-configure

Conversation

@connorjward
Copy link
Copy Markdown
Contributor

All logic is now gone. All possible configs are explicitly enumerated.

This is needed because the previous approach was not extensible (e.g.
supporting Ubuntu 24.04 and 26.04 together).

This opens the door to having 'community' configurations and even
externally provided configuration files.

All logic is now gone. All possible configs are explicitly enumerated.

This is needed because the previous approach was not extensible (e.g.
supporting Ubuntu 24.04 and 26.04 together).

This opens the door to having 'community' configurations and even
externally provided configuration files.
@connorjward connorjward added macOS gpu For special runner labels May 1, 2026
@connorjward connorjward marked this pull request as ready for review May 2, 2026 14:04
Comment thread scripts/firedrake-configure Outdated
Comment on lines +80 to +84
if args.gpu_arch != GPUPlatform.NO_GPU and os_ == OS.MACOS_ARM64:
error(
"GPU-compatible PETSc builds are currently only supported"
"on Linux"
)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should the test be for os_ != <LINUX_VALUE> rather than for MACOS?

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've removed this as the default error message is quite informative. I don't think we need a GPU-specific one.

Comment thread scripts/firedrake-configure Outdated
Comment thread scripts/firedrake-configure Outdated

def has_apt() -> bool:
try:
subprocess.run(["apt", "--version"], capture_output=True)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

which apt and check if it's empty rather than relying on errors?

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've dropped this and am detecting Ubuntu directly

Comment on lines +271 to +274
extra_url: str | None = None
"""The URL to a deb package that will enable vendor-specific software development
repositories, or None if not required.
"""
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is useful. Please can we also have docstrings for the other attributes?

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.

Done

Comment thread scripts/firedrake-configure Outdated
Comment on lines +287 to +288
"Don't know what system dependencies to install on this platform, "
"please install them manually"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should print some description of the Arch (e.g. name, platform, etc) so people know if they actually selected the one they thought they did.

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.

Done

Comment thread scripts/firedrake-configure Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gpu For special runner macOS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants