这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@inetol
Copy link
Member

@inetol inetol commented May 2, 2025

What does this PR do?

This is one of various PR to refactor the entire SearXNG Docker workflow.

Refactors integration.yml and moves container related things into container.yml, also touches needed updates into script files.

  • Podman is now supported to build the container images (Docker also received a refactor, merging both build and buildx)
  • Bringing massive reduction on runtime across all CI/CD workflows.
  • Container images are being built by Buildah instead of Docker BuildKit.
  • Container images are being tested before release.
  • Splitting "modern" (amd64 & arm64) and "legacy" (armv7) arches on different Dockerfiles to reduce dependency burden and allowing future image optimizations.

Why is this change important?

Same as #4699

How to test this PR locally?

make container.build

Author's checklist

We can't verify locally that the CI process works as expected, especially the container image release process. Due to the changes being relatively large, it's likely that some workflow will not work correctly the first time.

I try as much as possible to verify that everything works individually, but no promises, CI stuff.

@inetol

This comment was marked as resolved.

@return42
Copy link
Member

return42 commented May 3, 2025

Due to my limited access to the org .. I try as much as possible to verify that everything works individually, but no promises, CI stuff.

Thank you very much for the work you put in here ... if you get stuck at some point, don't hesitate to ask the core team for support. I myself am weak on the topics of GH actions and docker ... but there are others in our team and as far as I know they are very familiar with them ... and can also decide whether we should grant you additional rights on the GH repository.

@inetol inetol mentioned this pull request May 4, 2025
@inetol inetol changed the title Rework Docker release workflow Rework CI/CD workflows May 4, 2025
@unixfox
Copy link
Contributor

unixfox commented May 4, 2025

Just would like to point out that every commit creates a notification on our side. It would maybe be interesting to test in a fork on your side. Thanks

@inetol
Copy link
Member Author

inetol commented May 4, 2025

Just would like to point out that every commit creates a notification on our side. It would maybe be interesting to test in a fork on your side. Thanks

Sorry about that, I will try to move the tests directly to the fork, but I don't know if the CI will work as expected.

@inetol
Copy link
Member Author

inetol commented May 7, 2025

There should be no more major changes to this PR unless requested. I have updated the PR information with the most significant changes #4707 (comment)

Take your time to read the diff and request changes.

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 refactors the CI/CD workflows and reworks the container build process to support Podman and Buildah while improving runtime efficiency. Key changes include a redesigned container script (lib_sxng_container.sh) to merge Docker and Podman builds, updates to workflow YAML files for enhanced caching and concurrent execution, and adjustments in version handling within searx/version.py.

Reviewed Changes

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

File Description
utils/lib_sxng_container.sh Refactored container build, test, and push functions; improved engine selection and architecture handling
searx/version.py Updated version resolution to handle GitHub Actions environment variables
manage, Makefile, Dockerfiles, workflows Updated command aliases, caching keys, and YAML definitions to streamline CI/CD processes

@inetol inetol changed the title Rework CI/CD workflows [enh] Rework CI/CD workflows May 7, 2025
Comment on lines 264 to +277
case "$cmd" in
--getenv) var="$1"; echo "${!var}";;
--help) help;;
--*)
help
err_msg "unknown option $cmd"
--getenv)
var="$1"
echo "${!var}"
;;
--help) help ;;
--*)
help
err_msg "unknown option $cmd"
return 42
;;
*)
_type="$(type -t "$cmd")"
if [ "$_type" != 'function' ]; then
Copy link
Member

Choose a reason for hiding this comment

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

I think these modifications here are only whitespaces .. may related to your editor settings. Some editors interpret one tab as 4 spaces (a tab is by definition 8 spaces). To avoid tab problem, we don't use tab for indentation (except in Makefiles, where a tab is needed).

For shell files we use 4 spaces for one indentation:

searxng/.editorconfig

Lines 5 to 7 in bc06b1a

[*]
indent_style = space
indent_size = 4

Can you please check your editor settings.

Copy link
Member Author

Choose a reason for hiding this comment

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

The editor forces me to use the style defined in .editorconfig and indeed those are spaces and not tabs.

What I do use is https://github.com/mvdan/sh and it seems that by default it flattens the case branches, that's why the diff

Comment on lines 160 to 169
PLATFORM="$(python -c 'import platform; print(platform.system().lower(), platform.architecture()[0])')"
case "$PLATFORM" in
"linux 32bit" | "linux2 32bit") ARCH="linux32";;
"linux 64bit" | "linux2 64bit") ARCH="linux64";;
"windows 32 bit") ARCH="win32";;
"windows 64 bit") ARCH="win64";;
"mac 64bit") ARCH="macos";;
"linux 32bit" | "linux2 32bit") ARCH="linux32" ;;
"linux 64bit" | "linux2 64bit") ARCH="linux64" ;;
"windows 32 bit") ARCH="win32" ;;
"windows 64 bit") ARCH="win64" ;;
"mac 64bit") ARCH="macos" ;;
esac
GECKODRIVER_URL="https://github.com/mozilla/geckodriver/releases/download/$GECKODRIVER_VERSION/geckodriver-$GECKODRIVER_VERSION-$ARCH.tar.gz";
GECKODRIVER_URL="https://github.com/mozilla/geckodriver/releases/download/$GECKODRIVER_VERSION/geckodriver-$GECKODRIVER_VERSION-$ARCH.tar.gz"

Copy link
Member

Choose a reason for hiding this comment

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

🤔 do you use a code optimizer tool / code formating tool ? --> except the removed docker function all other modifications in this script are not needed.

If you know a code formating tool for shell scripts, and you think its worth to add to our tool chain, then please open a new PR for such a task.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should I revert those style change for now? I'll open a PR later with the tool.

Copy link
Member

Choose a reason for hiding this comment

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

Should I revert those style change for now?

Yes, it is better to not have this modification in this PR / they are not needed to achieve the goal of this PR.

I'll open a PR later with the tool.

👍

@return42
Copy link
Member

return42 commented May 8, 2025

The PR brings many valuable changes, but it also combines too many changes. It will therefore be difficult for any reviewer to approve this PR in its entirety.

I would suggest creating smaller PRs that cherry pick topics from this DRAFT. I would therefore suggest to create

  • one PR for the build of the online doc (documentation.yml)
  • one PR for translation tasks (l10n.yml)
  • one PR for docker build tasks (the name container.yml does not fit, since it is not about containers in general but docker in particular)
  • ...

@return42
Copy link
Member

return42 commented May 8, 2025

TL;DR / the follwing rules should be in mind, when creating a commit:

  • commit history should be read like a history book
  • commit messages are for the reader not for the author of the commit

There should be no more major changes to this PR unless requested.

This PR has 57 commits .. but not a single one is related to a separate topic ... some commit messages have titles like . or .. ... strictly speaking they are not commits but saves of files (like: "save my files to the repo").

A commit should always have a context and the commit message describes what is to be changed in that context, just as a function description should describe what the intention and the goal of the function is, a commit message should describe what the intention and the goal of that commit is.

The commits are not used to save files.

The commit messages form the history and are the first and therefore most important information a developer has when he has to research when and why a change had to be made and how it was made (what the goal was).

Like any text, a commit message should be written for the reader and not from the perspective of the author.

When scrolling through the history, the first thing one see is the title of the commit message. Therefore the title should describe the change as briefly and precisely as possible ... followed by a blank line and then a somewhat detailed description of the change.


Since the 57 commits are of no further significance, they could be combined into one commit ... or you could take the trouble to divide them into topics, but since we probably have to make new (individual) PRs for the topics anyway, we can improve the commit history even then.

@inetol
Copy link
Member Author

inetol commented May 8, 2025

This PR has 57 commits .. but not a single one is related to a separate topic ... some commit messages have titles like . or .. ... strictly speaking they are not commits but saves of files (like: "save my files to the repo").

Yeah, most of them come from another branch of this fork. I'll squash and document them now.

inetol added 6 commits May 8, 2025 11:35
Also fixes a cache issue found on data-update.yml, this fix is applied on checker.yml and container.yml files
Again. Needed to test the workflows in a separate fork so unrelated triggers appear in files. Also, adds the new Dockerfile.compat for armv7.
inetol added 6 commits May 8, 2025 11:44
Needed to test the workflows in a separate fork so unrelated triggers appear in files.

Run tests before container release, if there are any problems the job fails and the images are not released. At the moment this is basic, but it should cover the most serious issues that may appear.

This will prevent fiascos like searxng#4718 to ever happen again (hopefully)
concurrency rules applies to each branch
When running the version.py in CI, it only fetches a detached HEAD failing to get the reference. As I do not know very well if modifying the function will affect "normal" cases, I have preferred to use the variables provided by GHA to obtain the branch and the location of the repository.
preparation for initial review, moves all container stuff to lib_sxng_container.sh and reverts changes from fork CI testing
@inetol inetol removed the request for review from dalf May 8, 2025 10:22
@inetol inetol changed the title [enh] Rework CI/CD workflows [mod] rework container CI/CD May 8, 2025
checker:
name: Checker
runs-on: ubuntu-24.04
search:
Copy link
Contributor

Choose a reason for hiding this comment

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

Note:

  • the Python module name should be searx.checker rather than searx.search.checker.
  • for a time it made sense to embed this tool in the Flask app, I think now it could move to external tool.

@@ -0,0 +1,106 @@
FROM docker.io/library/python:3.13-slim AS builder
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be common knowledge, but adding a comment at the top of the file to indicate that this Dockerfile is intended for ARMv7 would be helpful for new comers.

If we want to avoid to duplicate information (ARMv7 here in comment, ARMv7 in code elsewhere), we can add a comment stating that utils/lib_sxng_container.sh pick this file for some architectures.

Copy link
Member Author

Choose a reason for hiding this comment

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

@inetol
Copy link
Member Author

inetol commented May 10, 2025

I'll keep the commit devel history here.

This PR is superseded by #4764

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.

4 participants