Skip to content
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

posix.redirect2null deprecated without replacement #3192

Closed
fweimer-rh opened this issue Jul 3, 2024 · 8 comments · Fixed by #3241
Closed

posix.redirect2null deprecated without replacement #3192

fweimer-rh opened this issue Jul 3, 2024 · 8 comments · Fixed by #3241
Assignees
Labels
lua Lua bindings/interface RFE
Milestone

Comments

@fweimer-rh
Copy link

Describe the bug
Commit 46bd0ed added a deprecation warning to posix.redirect2null and others, pointing to rpm.execute as a replacement, but rpm.execute does not provide a way to suppress output.

Expected behavior
There is a way to run a program with its output suppressed.

We'd use this in the Fedora glibc package because we want to suppress the error message from systemctl daemon-reexec if systemd is not running. Bash is not necessarily installed at this point.

@dmnks dmnks added this to the 4.20.0 milestone Jul 3, 2024
@pmatilai pmatilai added RFE lua Lua bindings/interface labels Jul 29, 2024
@pmatilai
Copy link
Member

pmatilai commented Jul 29, 2024

It's unfortunate that nobody saw this coming wrt rpm.execute().

I guess it's a matter of a few posix_spawn_file_actions_*() calls but the big question is what (and how) to expose on rpm.execute() level. If we previously had a special function for redirecting to null then maybe it could be something like two optional extra args to silence stdout and/or stderr, even if it seems a bit limiting.

@fweimer-rh
Copy link
Author

You could add a Lua table argument with some sort of encoding of the posix_spawn actions. This way, you could extend things further if new actions are added.

@pmatilai pmatilai self-assigned this Aug 7, 2024
@pmatilai
Copy link
Member

pmatilai commented Aug 9, 2024

Possible, sure. To me it's more of a question how to do it in a nice way.

First of all it basically requires adding an alternative syntax of rpm.execute() that accepts the command and its arguments as a table, the current syntax is not enhanceable because it's defined as all arguments to be passed to the command itself. Adding this table form isn't hard or a bad thing at all, but it's a pre-requisite nevertheless. Then we could add eg file actions as an optional table of tables argument.

One problem with extending rpm.execute() is that it'd be hard/impossible to detect whether the current rpm version supports this new syntax, so maybe this should be a new function instead 🤔 OTOH, I spot exactly one user of rpm.execute() in Fedora package set, with glibc being a potential second, and two users of redirect2null(). So it seems the clientele here is a highly specialized group of packages in the early bootstrap set, and others are just using os.execute().

Another issue here is that exposing the raw posix fileactions seems fiddly and ugly for the alleged user of rpm.execute() who just wants to call a damn helper (but, sooner or later you need output control). And given the way you're supposed to pass exact descriptor number to the low-level actions, wrapping it all in a higher level action (eg redirect fd to /dev/null) seems problematic.

@pmatilai
Copy link
Member

pmatilai commented Aug 9, 2024

#3236 adds the alternative table based calling mode as a pre-requisite to this.

@pmatilai
Copy link
Member

pmatilai commented Aug 9, 2024

After letting the "downstairs guys" chew on this while I had lunch, I think the kind of interface rpm.execute() wants is simply two optional extra arguments for redirecting stdout and stderr, respectively (for the variant that takes the command and options as a table). If it one day turns out that we need something fancier, we can wrap the full posix_spawn() API and call it thus.

@pmatilai
Copy link
Member

pmatilai commented Aug 9, 2024

A simple path-based redirection added in the linked PR now. It's simple for simple things but might be too limiting for something more advanced. Comments + suggestions welcome.

pmatilai added a commit to pmatilai/rpm that referenced this issue Aug 13, 2024
Turns out real-world usage needs more control than rpm.execute()
delivers. This could be crammed into rpm.execute() but it'd be forward
incompatible in a somewhat non-obvious way, we might just as well
add a separate function for it.

Supports redirecting stdin, stdout and stderr to a given path, support
for file descriptors, other actions and spawn attributes can be added
later.

Fixes: rpm-software-management#3192
pmatilai added a commit to pmatilai/rpm that referenced this issue Aug 13, 2024
Turns out real-world usage needs more control than rpm.execute()
delivers. This could be crammed into rpm.execute() but it'd be forward
incompatible in a somewhat non-obvious way, we might just as well
add a separate function for it.

Supports redirecting stdin, stdout and stderr to a given path, support
for file descriptors, other actions and spawn attributes can be added
later.

Fixes: rpm-software-management#3192
pmatilai added a commit to pmatilai/rpm that referenced this issue Aug 13, 2024
Turns out real-world usage needs more control than rpm.execute()
delivers. This could be crammed into rpm.execute() but it'd be forward
incompatible in a somewhat non-obvious way, we might just as well
add a separate function for it.

Supports redirecting stdin, stdout and stderr to a given path, support
for file descriptors, other actions and spawn attributes can be added
later.

Fixes: rpm-software-management#3192
pmatilai added a commit to pmatilai/rpm that referenced this issue Aug 13, 2024
Turns out real-world usage needs more control than rpm.execute()
delivers. This could be crammed into rpm.execute() but it'd be forward
incompatible in a somewhat non-obvious way, we might just as well
add a separate function for it.

Supports redirecting stdin, stdout and stderr to a given path, support
for file descriptors, other actions and spawn attributes can be added
later.

Fixes: rpm-software-management#3192
pmatilai added a commit to pmatilai/rpm that referenced this issue Aug 16, 2024
Turns out real-world usage needs more control than rpm.execute()
delivers. This could be crammed into rpm.execute() but it'd be forward
incompatible in a somewhat non-obvious way, we might just as well
add a separate function for it.

Supports redirecting stdin, stdout and stderr to a given path, support
for file descriptors, other actions and spawn attributes can be added
later.

Fixes: rpm-software-management#3192
@pmatilai
Copy link
Member

On a related note, I just realized that we can't really remove these even in v6, because as rare as the use of these functions are, those uses are on some of the most central packages of distros and removing them would suddenly make all current Fedora/RHEL versions uninstallable. Which would be the single biggest incompatibility in v6, and that doesn't make any sense.

It'd be a whole lot more meaningful if we could issue those deprecation warnings on package build instead of install, but I don't see a way to do that. Another possibility is to only support these for v4 packages, somehow.

pmatilai added a commit to pmatilai/rpm that referenced this issue Aug 22, 2024
Turns out real-world usage needs more control than rpm.execute()
delivers. This could be crammed into rpm.execute() but it'd be forward
incompatible in a somewhat non-obvious way, we might just as well
add a separate function for it.

Supports redirecting stdin, stdout and stderr to a given path, support
for file descriptors, other actions and spawn attributes can be added
later.

Fixes: rpm-software-management#3192
pmatilai added a commit to pmatilai/rpm that referenced this issue Aug 22, 2024
Turns out real-world usage needs more control than rpm.execute()
delivers. This could be crammed into rpm.execute() but it'd be forward
incompatible in a somewhat non-obvious way, we might just as well
add a separate function for it.

Supports redirecting stdin, stdout and stderr to a given path, support
for file descriptors, other actions and spawn attributes can be added
later.

Fixes: rpm-software-management#3192
dmnks pushed a commit that referenced this issue Aug 27, 2024
Turns out real-world usage needs more control than rpm.execute()
delivers. This could be crammed into rpm.execute() but it'd be forward
incompatible in a somewhat non-obvious way, we might just as well
add a separate function for it.

Supports redirecting stdin, stdout and stderr to a given path, support
for file descriptors, other actions and spawn attributes can be added
later.

Fixes: #3192
dmnks pushed a commit to dmnks/rpm that referenced this issue Aug 29, 2024
Turns out real-world usage needs more control than rpm.execute()
delivers. This could be crammed into rpm.execute() but it'd be forward
incompatible in a somewhat non-obvious way, we might just as well
add a separate function for it.

Supports redirecting stdin, stdout and stderr to a given path, support
for file descriptors, other actions and spawn attributes can be added
later.

Fixes: rpm-software-management#3192
(cherry picked from commit 6444f71)
dmnks pushed a commit that referenced this issue Aug 30, 2024
Turns out real-world usage needs more control than rpm.execute()
delivers. This could be crammed into rpm.execute() but it'd be forward
incompatible in a somewhat non-obvious way, we might just as well
add a separate function for it.

Supports redirecting stdin, stdout and stderr to a given path, support
for file descriptors, other actions and spawn attributes can be added
later.

Fixes: #3192
(cherry picked from commit 6444f71)
@pmatilai
Copy link
Member

pmatilai commented Sep 3, 2024

On a related note, here's an attempt to make the deprecation warnings less obnoxious, ie avoid them on old packages: #3270

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lua Lua bindings/interface RFE
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants