Skip to content

Support Sisco #17

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
gmloose wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Support Sisco #17

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
f5de9e8
Support Sisco
gmloose Jun 22, 2026
1b605d9
Add missing option
gmloose Jun 22, 2026
878e979
Fix Formula
gmloose Jun 22, 2026
e4c217f
Remove condition test for sisco
gmloose Jun 22, 2026
0383a70
Move conditional package to the end
gmloose Jun 22, 2026
592cd70
Change option to without-sisco
gmloose Jun 22, 2026
73f5fb2
Bump Ubuntu and MacOS versions
gmloose Jun 22, 2026
0de6e01
Bump version of old GitHub actions
gmloose Jun 22, 2026
59058b5
Merge branch 'bump-ubuntu-and-macos-versions' into support-sisco
gmloose Jun 22, 2026
a58be0c
Revert "Change option to without-sisco"
gmloose Jun 22, 2026
58f1815
Switch default branch for Homebrew/actions
gmloose Jun 22, 2026
cec054c
Fix default branch for all Homebrew/actions
gmloose Jun 22, 2026
e7e9a53
Reapply "Change option to without-sisco"
gmloose Jun 22, 2026
efd854b
Revert Formula changes, just add libdeflate dependency
gmloose Jun 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions .github/workflows/publish.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,16 @@ on:
jobs:
pr-pull:
if: contains(github.event.pull_request.labels.*.name, 'pr-pull')
runs-on: ubuntu-22.04
runs-on: ubuntu-24.04
permissions:
contents: write
pull-requests: write
steps:
- name: Set up Homebrew
uses: Homebrew/actions/setup-homebrew@master
uses: Homebrew/actions/setup-homebrew@main

- name: Set up git
uses: Homebrew/actions/git-user-config@master
uses: Homebrew/actions/git-user-config@main

- name: Pull bottles
env:
Expand All @@ -26,7 +26,7 @@ jobs:
run: brew pr-pull --debug --tap="$GITHUB_REPOSITORY" "$PULL_REQUEST"

- name: Push commits
uses: Homebrew/actions/git-try-push@master
uses: Homebrew/actions/git-try-push@main
with:
token: ${{ github.token }}
branch: master
Expand Down
8 changes: 4 additions & 4 deletions .github/workflows/tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,15 @@ jobs:
test-bot:
strategy:
matrix:
os: [ubuntu-22.04, macos-14]
os: [ubuntu-24.04, macos-15]
runs-on: ${{ matrix.os }}
steps:
- name: Set up Homebrew
id: set-up-homebrew
uses: Homebrew/actions/setup-homebrew@master
uses: Homebrew/actions/setup-homebrew@main

- name: Cache Homebrew Bundler RubyGems
uses: actions/cache@v4
uses: actions/cache@v5
with:
path: ${{ steps.set-up-homebrew.outputs.gems-path }}
key: ${{ runner.os }}-rubygems-${{ steps.set-up-homebrew.outputs.gems-hash }}
Expand All @@ -35,7 +35,7 @@ jobs:

- name: Upload bottles as artifact
if: always() && github.event_name == 'pull_request'
uses: actions/upload-artifact@v4
uses: actions/upload-artifact@v7
with:
name: bottles_${{ matrix.os }}
path: '*.bottle.*'
5 changes: 5 additions & 0 deletions Formula/casacore.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,10 @@ class Casacore < Formula
head "https://github.com/casacore/casacore.git"

option "without-python", "Build without Python bindings"
option "without-sisco", "Build without Sisco compression storage manager"

@gmloose gmloose Jun 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Also I don't know exactly how this works. Whether this means "with" or "without" if nothing is specified on the command line. I'm not even sure we need all this if we enable Sisco by default in the casacore build. Then probably just the following single line suffices:

depends_on "libdeflate"

And we don't need to add the CMake argument on line 52 either. What do you think?

@tammojan tammojan Jun 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I suggest to make the dependency non-optional, and just build sisco always. There are many other parts of casacore that could be built optional in homebrew and we don't do that either.

@gmloose gmloose Jun 23, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

OK, so shall I then convert this PR to just a one-liner that adds "libdeflate" as dependency?

@tammojan tammojan Jun 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes please


depends_on "cmake" => :build

depends_on "casacore-data"
depends_on "cfitsio"
depends_on "fftw"
Expand All @@ -19,6 +21,8 @@ class Casacore < Formula
depends_on "readline"
depends_on "wcslib"

depends_on "libdeflate" => :optional

@tammojan tammojan Jun 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't know what this optional does, I don't think you've told homebrew that the libdeflate dependency is related to sisco. Since libdeflate is in homebrew, I would be fine with making the dependency non-optional.

@gmloose gmloose Jun 23, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This seems to be the modern way (according to ChatGPT) to use a conditional. I first tried:

    if build.with?("sisco")
        depends_on("libdeflate")
    endif

But I received errors and warning that I didn't understand. The version with ":optional" passed the test.


if build.with?("python")
depends_on "python3"
depends_on "numpy"
Expand All @@ -45,6 +49,7 @@ def install
numpy_include = `#{python_exe} -c "import numpy; print(numpy.get_include())"`.strip
cmake_args << "-DPython3_NumPy_INCLUDE_DIR=#{numpy_include}"
end
cmake_args << "-DBUILD_SISCO=#{build.with?("sisco") ? "ON" : "OFF"}"
system "cmake", "../..", *cmake_args
system "make", "install"
end
Expand Down
Loading