-
Notifications
You must be signed in to change notification settings - Fork 31
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.
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
ostree: move admindir to /etc/alternatives.admindir #134
Conversation
@cgwalters @Conan-Kudo @travier @mrguitar what do you think? |
I think one more option could be to auto-detect running on an ostree-based system and then use the "new" path. |
70e18ae
to
d31c62f
Compare
Fixed the build warnings by removing redundantly listed files in the .spec. CI didn't fire for 6 months. |
You'll probably want to use another folder instead of a subfolder to avoid any potential name conflict with existing configs. As for the migration, one option would be to migrate the config from |
That's probably a good option as well. It would have to:
|
po/chkconfig.pot
Outdated
@@ -8,7 +8,7 @@ msgid "" | |||
msgstr "" | |||
"Project-Id-Version: PACKAGE VERSION\n" | |||
"Report-Msgid-Bugs-To: \n" | |||
"POT-Creation-Date: 2024-01-09 12:58+0100\n" | |||
"POT-Creation-Date: 2024-07-24 11:00+0200\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spurious change; I think gettext will do this by default unfortunately. Could probably break out "update gettext" into a distinct PR, otherwise this would (in the extremely unlikely event someone did a different PR) create unnecessary conflicts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will update before (potentially) merging
Or, rely on an environment variable and/or config file to opt-in? The file path could literally look like: |
Note that this would probably need to be advertised as a Fedora Change if we don't constraint it only to ostree based systems (but even in this case too as someone could have setup things this ways already). The deadline for submitting a change for F41 is passed but it could be considered for an exception. |
I want to constrain it to ostree systems only. I am not sure how somebody would have set things up already; it didn't work before. Somebody could have used the CLI flag to change the path but this would continue to work. |
Small name bikeshedding note: I prefer Another option, not touching code, would be to do the migration in the %pre scriptlet + a systemd unit at boot time. In the
The systemd unit at boot time would trigger on |
You could have manually written your own alternatives config in |
If /var/lib/alternatives exists, we need to use it. |
d31c62f
to
cd1ce37
Compare
@lnykryn can you take a look? |
Went with calling it |
cd1ce37
to
a3468c0
Compare
Dockerfile to test: FROM quay.io/fedora/fedora-bootc:40
RUN mkdir -p /etc/alternatives.admindir
COPY alternatives /usr/sbin/alternatives
RUN dnf -y install golang
RUN ls /etc/.alternatives.admindir
RUN go help > /dev/null |
Looking at the failures, there's a ton of unrelated warnings that are mostly around missing error checking from e.g. There's clearly some UTF-8 corruption going on (I blame Python) but the fatal error is:
Ahhh...heh! So meta! Actually I think this PR is breaking installation of the linker, see this from the buildroot log:
(Though now I'm a bit confused since that's the buildroot install, how are we already getting the built binary there?) |
The weird thing is that it worked before when using |
How can it even try to use |
I don't think it should be a hidden dir. It's a "normal" config dir, just with a weird format. |
Not sure I understand here. My suggestion is to migrate the content from |
Roger that, will update.
I am afraid of a breaking change. Some users/workloads may depend (for whatever reason) on the content being in /var/lib/alternatives. Since alternatives never worked on ostree-based systems, we can create a new destination there while keeping the old behavior on non-ostree systems. That should avoid any breaking change to existing users. |
a3468c0
to
0a38c89
Compare
b60ac4d
to
261c2b2
Compare
Unfortunately, I heard about customers that are modifying /var/lib/alternatives with customer scripts (which is terrible, but what we can do). Generally, this approach makes sense, but I need some extra time to think it thoroughly through. |
@@ -81,9 +81,7 @@ mkdir -p $RPM_BUILD_ROOT/etc/chkconfig.d | |||
%{_sysconfdir}/chkconfig.d | |||
%{_sysconfdir}/init.d | |||
%{_sysconfdir}/rc.d | |||
%{_sysconfdir}/rc.d/init.d |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this. If you also need some changes in chkconfig packaging, please create a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Builds won't pass otherwise. OK to break it out into a separate commit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should figure out what is wrong here, /etc/rc.d/init.d/ is the proper dir for initscripts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already %dir /etc/rc.d
so it complains about init.d
being listed redundantly
261c2b2
to
d55ac12
Compare
I believe something's off in the CI. I still see |
Yeah, I saw this in the past, the problem is that, for some reason, packit creates one repo for all the builds with the changes from PR and uses the repo also when installing the build root. And ld is in buildroot and ld uses alternatives. Try one more force push. |
d55ac12
to
b2ce82c
Compare
Also, let's remove creating ALTDIR = /var/lib/alternatives in the Makefile and instead create the admin directory directly in the alternative binary on the first use (based on the detected system). Hopefully, there won't be any SELinux problem. |
`ostree container commit` wipes /var and thereby erases the data in /var/lib/alternatives; the directory used to store configs/symlinks of alternatives. /var is not a good place for storing such configs since it won't receive updates on bootc images either. We need to move to another location. Hence, use /etc/alternatives.admindir when running on an ostree-based system unless /var/lib/alternatives is already present; a user may have worked around the problem. This way we enable alternatives to work on ostree-based systems without breaking backwards compat. Fixes: fedora-sysv#9 Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
b2ce82c
to
8837cde
Compare
Doesn't seem to work. |
Can we break that into a separate PR? I am afraid this may turn into a bigger change if others want to have /etc/alternatives also create on-demand. |
Continuing in #135 due the build root issue. Apologies for the inconvenience. |
Now added to the new PR. |
Wow...bonkers |
ostree container commit
wipes /var and thereby erases the data in/var/lib/alternatives; the directory used to store configs/symlinks of
alternatives.
/var is not a good place for storing such configs since it won't receive
updates on bootc images either. We need to move to another location.
Hence, use /etc/alternatives.admindir when running on an ostree-based
system unless /var/lib/alternatives is already present; a user may have
worked around the problem. This way we enable alternatives to work on
ostree-based systems without breaking backwards compat.
Fixes: #9
Signed-off-by: Valentin Rothberg vrothberg@redhat.com