Skip to content

Conversation

@arrowd
Copy link
Contributor

@arrowd arrowd commented Jan 21, 2026

When sourcing this file it may overwrite the VERSION shell variable, which in turn end up being used for @Version@ substitutions in various .in files.

I was lucky to catch this when preparing a FreeBSD port update.
Spec reference: https://www.freedesktop.org/software/systemd/man/latest/os-release.html#VERSION=

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

The pull request correctly identifies and fixes an issue where sourcing /etc/os-release can clobber the VERSION variable. However, the fix can be made more robust and portable. My review includes a suggestion to use a subshell to source /etc/os-release, which prevents clobbering of any shell variables, not just VERSION. This approach is cleaner and safer. Additionally, I've corrected a non-POSIX [[ test to a portable [ test, which was a pre-existing bug in this code block.

Copy link
Contributor

@thalman thalman left a comment

Choose a reason for hiding this comment

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

Please take a look at my comment

@arrowd arrowd force-pushed the platform-os-release branch from ef58252 to 0f3e51a Compare January 29, 2026 15:00
Copy link
Contributor

@thalman thalman left a comment

Choose a reason for hiding this comment

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

OK, somehow I overlooked that change, sorry and thanks.

@thalman thalman added no-backport This should go to target branch only. Accepted labels Jan 30, 2026
When sourcing this file it may overwrite the VERSION shell variable, which
in turn end up being used for @Version@ substitutions in various .in files.

While here, make sure we always set $osname to something sensible too.

Reviewed-by: Pavel Březina <pbrezina@redhat.com>
Reviewed-by: Tomáš Halman <thalman@redhat.com>
@sssd-bot
Copy link
Contributor

The pull request was accepted by @thalman with the following PR CI status:


🟢 CodeQL (success)
🟢 osh-diff-scan:fedora-rawhide-x86_64:upstream (success)
🟢 rpm-build:centos-stream-10-x86_64:upstream (success)
🟢 rpm-build:fedora-42-x86_64:upstream (success)
🟢 rpm-build:fedora-43-x86_64:upstream (success)
🟢 rpm-build:fedora-rawhide-x86_64:upstream (success)
🟢 Analyze (target) / cppcheck (success)
🟢 Build / freebsd (success)
🟢 Build / make-distcheck (success)
🟢 ci / intgcheck (centos-10) (success)
🟢 ci / intgcheck (fedora-42) (success)
🟢 ci / intgcheck (fedora-43) (success)
🟢 ci / intgcheck (fedora-44) (success)
🟢 ci / prepare (success)
🔴 ci / system (centos-10) (failure)
🔴 ci / system (fedora-42) (failure)
🟢 ci / system (fedora-43) (success)
🔴 ci / system (fedora-44) (failure)
➖ Coverity scan / coverity (skipped)
🟢 Static code analysis / codeql (success)
🟢 Static code analysis / pre-commit (success)
🟢 Static code analysis / python-system-tests (success)


There are unsuccessful or unfinished checks. Make sure that the failures are not related to this pull request before merging.

@sssd-bot sssd-bot force-pushed the platform-os-release branch from 0f3e51a to 0b54224 Compare January 30, 2026 15:44
@alexey-tikhonov alexey-tikhonov merged commit 308af8f into SSSD:master Jan 31, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Accepted no-backport This should go to target branch only.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants