Skip to content

Conversation

@zanchey
Copy link
Member

@zanchey zanchey commented Oct 5, 2025

Test this actually works on Cirrus

@zanchey zanchey force-pushed the fish-reenable-cirrus branch from 631fe74 to bd0af5d Compare October 6, 2025 02:26
krobelus added a commit that referenced this pull request Oct 6, 2025
These should work now that we have (automatically-updated) docker
builds via .github/workflows/docker_builds.yml.

Ref: #11884 (comment)
krobelus added a commit that referenced this pull request Oct 6, 2025
These should work now that we have (automatically-updated) docker
builds via .github/workflows/docker_builds.yml.

Ref: #11884 (comment)
krobelus added a commit to krobelus/fish-shell that referenced this pull request Oct 6, 2025
Else this runs when people push to their master's forks, see
fish-shell#11884 (comment)
krobelus added a commit to krobelus/fish-shell that referenced this pull request Oct 6, 2025
These should work now that we have (automatically-updated) docker
builds via .github/workflows/docker_builds.yml.

Ref: fish-shell#11884 (comment)
krobelus added a commit to krobelus/fish-shell that referenced this pull request Oct 6, 2025
The root cause was that FISH_CHECK_LINT was not set. By
default, check.sh should fail if any tool is not installed, see
59b4398 (build_tools/style.fish: fail if formatters are not
available, 2025-07-24); like it does for rustfmt and clippy.

This reverts commit fbfd29d.

See fish-shell#11884
krobelus added a commit to krobelus/fish-shell that referenced this pull request Oct 6, 2025
I think we do want to stop using CMake on Cirrus but this should
first be tested in combination with all the other changes that made
it to master concurrently (test by pushing a temporary branch to the
fish-shell repo), to avoid confusion as to what exactly broke.

This reverts commit d167ab9.

See fish-shell#11884
krobelus added a commit to krobelus/fish-shell that referenced this pull request Oct 6, 2025
Else this runs when people push to their master's forks, see
fish-shell#11884 (comment)
@krobelus krobelus force-pushed the fish-reenable-cirrus branch from bd0af5d to f5cafe0 Compare October 6, 2025 11:41
krobelus added a commit that referenced this pull request Oct 6, 2025
These should work now that we have (automatically-updated) docker
builds via .github/workflows/docker_builds.yml.

Ref: #11884 (comment)
@krobelus
Copy link
Contributor

krobelus commented Oct 6, 2025

TODO: need to either re-revert b1e8fdf (Revert "CI: use build_tools/check.sh in Cirrus CI", 2025-10-06),
or keep using cmake (I have not had time to figure out what was the problem; some of them symptoms are documented in the commits I pushed to this PR).

The fact that .github/workflows/docker_builds.yml only writes to a central registry that's directly used by master is not ideal;
maybe we can set things up so it's easy to test everything in a temporary branch, e.g. by allowing workflow_dispatch triggers to use a custom docker registry namespace

krobelus added a commit that referenced this pull request Oct 7, 2025
These should work now that we have (automatically-updated) docker
builds via .github/workflows/docker_builds.yml.

Ref: #11884 (comment)
@krobelus krobelus force-pushed the fish-reenable-cirrus branch from f5cafe0 to da0d6b8 Compare October 7, 2025 14:19
@zanchey
Copy link
Member Author

zanchey commented Oct 7, 2025

The fact that .github/workflows/docker_builds.yml only writes to a central registry that's directly used by master is not ideal; maybe we can set things up so it's easy to test everything in a temporary branch, e.g. by allowing workflow_dispatch triggers to use a custom docker registry namespace

You can test it in your own fork and it will write to your own package registry. A dispatch trigger would be a fine thing to add.

I was going to poke at this some more but it looks like you want to take it over?

@krobelus krobelus force-pushed the fish-reenable-cirrus branch from da0d6b8 to 4646331 Compare October 7, 2025 14:30
@krobelus
Copy link
Contributor

krobelus commented Oct 7, 2025

You can test it in your own fork and it will write to your own package registry

right, that should work (probably some of the ones in https://github.com/krobelus/fish-shell/actions/runs/18277577848 failed because I had never pushed those images before, so I get a permissions error).

I was going to poke at this some more but it looks like you want to take it over?

Feel free to take it back (and drop the bad commit that's unrelated to Cirrus).
We could stick with cmake for now, to keep it close to current GitHub actions, but check.sh should work, and might be easier because I already removed CMake from the docker images

krobelus pushed a commit to krobelus/fish-shell that referenced this pull request Oct 12, 2025
krobelus added a commit to krobelus/fish-shell that referenced this pull request Oct 12, 2025
These should work now that we have (automatically-updated) docker
builds via .github/workflows/docker_builds.yml.

Ref: fish-shell#11884 (comment)
krobelus pushed a commit to krobelus/fish-shell that referenced this pull request Oct 12, 2025
krobelus added a commit to krobelus/fish-shell that referenced this pull request Oct 12, 2025
These should work now that we have (automatically-updated) docker
builds via .github/workflows/docker_builds.yml.

Ref: fish-shell#11884 (comment)
krobelus and others added 3 commits October 12, 2025 07:17
These should work now that we have (automatically-updated) docker
builds via .github/workflows/docker_builds.yml.

Ref: #11884 (comment)
This fails with "exec format error" because our container is built
on a 64 bit system on GitHub Actions.  Not yet sure how to fix that.
.cirrus.yml Outdated
# container:
# <<: *step
# image: ghcr.io/krobelus/fish-ci/focal-32bit:latest
# image: ghcr.io/fish-shell/fish-ci/focal-32bit:latest
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok it wasn't so bad; I only had to add write permissions on some of the docker images in my registry.
I'm more or less ready to push a version that has successfully passed tests on my fork.
Though I disabled jammy-armv7-32bit for now, due to exec format error.. maybe this container

          - os: ubuntu-24.04-arm
            target: jammy-armv7-32bit

needs to be built on a 32bit host?


Whenever we make a change to docker/ on master,
we need to first push those changes,
wait for the docker images to be pushed,
and then re-run CI.
If the previous push needs the new docker images, its CI run would fail.

It's much better than before, but ideally we can declare build-docker-images a prerequisite for .cirrus.yml,
so Cirrus waits for that. I wonder what's the best way to do that.
(One workaround is manual kickoff of build-docker-images on a non-master branch before pushing to master)

Copy link
Contributor

Choose a reason for hiding this comment

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

wait for the docker images to be pushed

pushed an untested draft commit for that.
Will need to test it with Cirrus on my fork,
and when that works add fish-shell's CIRRUS_TOKEN to github secrets.

@krobelus krobelus force-pushed the fish-reenable-cirrus branch from 4646331 to 8a4a8bb Compare October 12, 2025 06:35
(TODO this is totally untested and mostly vibe coded.)

Whenever we push changes do docker/**, our docker images for Cirrus
CI will be rebuilt.

However, the Cirrus CI jobs will kick off at the same time as the
Docker builds, so they will likely use old images.  This can cause
surprising (albeit transient) failures.

Fix this by having Cirrus wait for GitHub Actions.

This addresses the second part of
#11884 (comment)
krobelus added a commit to krobelus/fish-shell that referenced this pull request Oct 17, 2025
See fish-shell#11961 (comment)

I didn't find a good way to add ~/.cargo/bin to $PATH only for
fishuser, so hardcode it for all users for now.  The default rustup
behavior (source it in ~/.bashrc) only works for interactive shells,
so it doesn't work in CI.

I updated the images manually for now, because I don't want to test
on master (see fish-shell#11884 and
fish-shell#11961 (comment))

	# Upload new docker images.
	repo+owner=krobelus
	git push $repo_owner HEAD:tmp
	gh --repo=$repo_owner/fish-shell \
	    workflow run --ref=tmp \
	    .github/workflows/build_docker_images.yml
	# https://github.com/$repo_owner/fish-shell/actions/workflows/build_docker_images.yml
	sleep 3m
	# Enable Cirrus CI on fork.
	sed -i "s/OWNER == 'fish-shell'/OWNER == '$repo_owner'/g" .cirrus.yml
	git commit &&
	git push $repo_owner HEAD:tmp

Part of fish-shell#11961
krobelus added a commit to krobelus/fish-shell that referenced this pull request Oct 17, 2025
See fish-shell#11961 (comment)

I didn't find a good way to add ~/.cargo/bin to $PATH only for
fishuser, so hardcode it for all users for now.  The default rustup
behavior (source it in ~/.bashrc) only works for interactive shells,
so it doesn't work in CI.

I updated the images manually for now, because
I'm not sure I want to test on master (see
fish-shell#11884 and
fish-shell#11961 (comment))

	sed -i '/if: github.repository_owner/d' .github/workflows/build_docker_images.yml
	git commit
	# Upload new docker images.
	repo_owner=krobelus
	git push $repo_owner HEAD:tmp
	gh --repo=$repo_owner/fish-shell \
	    workflow run --ref=tmp \
	    .github/workflows/build_docker_images.yml
	# https://github.com/$repo_owner/fish-shell/actions/workflows/build_docker_images.yml
	sleep 3m
	# Enable Cirrus CI on fork.
	sed -i "s/OWNER == 'fish-shell'/OWNER == '$repo_owner'/g" .cirrus.yml
	git commit
	git push $repo_owner HEAD:tmp

Part of fish-shell#11961
krobelus added a commit to krobelus/fish-shell that referenced this pull request Oct 19, 2025
See fish-shell#11961 (comment)

I didn't find a good way to add ~/.cargo/bin to $PATH only for
fishuser, so hardcode it for all users for now.  The default rustup
behavior (source it in ~/.bashrc) only works for interactive shells,
so it doesn't work in CI.

I updated the images manually for now, because
I'm not sure I want to test on master (see
fish-shell#11884 and
fish-shell#11961 (comment))

	sed -i '/if: github.repository_owner/d' .github/workflows/build_docker_images.yml
	git commit
	# Upload new docker images.
	repo_owner=krobelus
	git push $repo_owner HEAD:tmp
	gh --repo=$repo_owner/fish-shell \
	    workflow run --ref=tmp \
	    .github/workflows/build_docker_images.yml
	# https://github.com/$repo_owner/fish-shell/actions/workflows/build_docker_images.yml
	sleep 3m
	# Enable Cirrus CI on fork.
	sed -i "s/OWNER == 'fish-shell'/OWNER == '$repo_owner'/g" .cirrus.yml
	git commit
	git push $repo_owner HEAD:tmp

Part of fish-shell#11961
krobelus added a commit to krobelus/fish-shell that referenced this pull request Nov 2, 2025
krobelus added a commit to krobelus/fish-shell that referenced this pull request Nov 2, 2025
krobelus added a commit to krobelus/fish-shell that referenced this pull request Nov 2, 2025
cirrus.yml depends on .github/workflows/build_docker_images.yml but
the CI doesn't know that.

Simplify things by building docker images directly in Cirrus CI
(following https://cirrus-ci.org/guide/docker-builder-vm/).  No need
to push them to a registry, Cirrus caches for us.

While at it, add back ARM jobs that should mostly match the ones
removed in c0b7167 (Remove unused docker images for frozen OS
releases, 2025-10-31). The exact Ubuntu versions are different,
but those are probably arbitrary.

Closes fish-shell#11884
Closes fish-shell#11871
krobelus added a commit to krobelus/fish-shell that referenced this pull request Nov 3, 2025
cirrus.yml depends on .github/workflows/build_docker_images.yml but
the CI doesn't know that.

Simplify things by building docker images directly in Cirrus CI
(following https://cirrus-ci.org/guide/docker-builder-vm/).  No need
to push them to a registry, Cirrus caches for us.

While at it, add back ARM jobs that should mostly match the ones
removed in c0b7167 (Remove unused docker images for frozen OS
releases, 2025-10-31). The exact Ubuntu versions are different,
but those are probably arbitrary.

Closes fish-shell#11884
Closes fish-shell#11871
krobelus added a commit that referenced this pull request Nov 3, 2025
cirrus.yml depends on .github/workflows/build_docker_images.yml but
the CI doesn't know that.

Simplify things by building docker images directly in Cirrus CI
(following https://cirrus-ci.org/guide/docker-builder-vm/).  No need
to push them to a registry, Cirrus caches for us.

While at it, add back ARM jobs that should mostly match the ones
removed in c0b7167 (Remove unused docker images for frozen OS
releases, 2025-10-31). The exact Ubuntu versions are different,
but those are probably arbitrary.

Closes #11884
Closes #11871
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.

3 participants