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

feat: allow passing G_SPAWN stdio flags to awesome.spawn #3932

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

aarondill
Copy link
Contributor

@aarondill aarondill commented Jun 30, 2024

Fixes: #3865
Currently works by allowing the exact strings "DEV_NULL" or "INHERIT" to be passed to return_std*.

@aarondill
Copy link
Contributor Author

I have no idea why the checks are failing to compile. This compiles (and runs successfully) locally. All the "missing" symbols should be provided by <glib.h>.

@aarondill
Copy link
Contributor Author

It turns out the GLIB Version is too low.
The source specifies that it's only available from version >=2.74; but this is not shown in the docs.
The gir1.2-gtk-3.0 Ubuntu package depends on GLib >= 1.39.0 (currently 1.64.0-2), so this isn't available on Ubuntu, while on Arch, I'm running Glib2 2.80.3-2.

@actionless
Copy link
Member

actionless commented Jul 2, 2024

prolly the only way for you here is to IFDEF it

and mention that in the docs (also you didn't updated the doc aside of changing the type)

@aarondill
Copy link
Contributor Author

@actionless I added a (compile-time) version check. I tried to use ifdef, but for some reason it wouldn't detect as defined.

@aarondill
Copy link
Contributor Author

The new behavior has been added to the documentation

spawn.c Outdated Show resolved Hide resolved
Copy link
Member

@actionless actionless left a comment

Choose a reason for hiding this comment

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

thanks 👍

@aarondill
Copy link
Contributor Author

Is there anything left for me to do for this to be merged, or are we just waiting for another review?

@actionless
Copy link
Member

see #3934

@Aire-One
Copy link
Member

There is also the Build & Test / fixed-lgi-lua5.2 check that is unsuccessful. So the automatic merge wouldn't work anyway here.

@aarondill
Copy link
Contributor Author

@Aire-One If I understand correctly, that check is testing each commit to check that they all pass tests?
Should I squash the commits together to ensure that the (now one) commit passes?

@actionless
Copy link
Member

yes

@aarondill
Copy link
Contributor Author

Alright, I've all the commits into the first (primary) commit.

@aarondill
Copy link
Contributor Author

From what I can tell, the CI checks failed during setup (and thusly not due to my code)

@Elv13
Copy link
Member

Elv13 commented Aug 20, 2024

Apparently this is documented incorrectly, but the second argument of awful.spawn can be a table or "arguments". Please use that table for the new features. Adding more and more argument to the function doesn't scale. You end up with awful.spawn("cmd", false, nil, nil, nil, nil "something"), which is hard to read. (edit: Note that this is for the awful.spawn part of this, not the spawn.c/awesome.spawn part of it, which can have whatever API, I don't really care since this is a super low level obscure API only really used internally).

Also, the oldest still supported glib2 version appears to be 2.56 (for RHEL 8.8). Ubuntu 20.04 and 22.04 also have older versions. So 2.74 definitively will prevent people from building on a large number of totally supported distributions. This will require some ifdef, otherwise this can't be merged. You can do them in CMake by adding a new -D, but using normal #ifdef provided by glib2 is preferred. Because it breaks the most used LTS distributions, raising the glib2 version this far cannot be done for many more years. The most used Linux distro in the world right now, while the number are hard to get, is most probably Ubuntu 22.04.

@aarondill
Copy link
Contributor Author

I currently haven't touched awful.spawn, are you saying you'd like me to add this functionality there (as well)?

I currently do have the >2.74 functionality behind an #if GLIB_CHECK_VERSION(2,74,0), which is removed in versions <2.74. According to the GLib docs, all the other flags are available since 2.0.

This absolutely should build in its current state, without raising the minimum required GLib version. If i'm mistaken and that's not the case, please let me know so I can fix it.

@aarondill
Copy link
Contributor Author

The CI builds and tests pass on GLib version 2.64.6; so i'm fairly certain that this change is backwards-compatible.

--   Found glib-2.0, version 2.64.6

@aarondill
Copy link
Contributor Author

see #3934

@actionless now that #3934 is fixed, are there any blockers to this pr?

Fixes: awesomeWM#3865
Currently works by allowing the exact strings "DEV_NULL" or "INHERIT" to
be passed to return_std*.

Signed-off-by: aarondill <[email protected]>
@aarondill
Copy link
Contributor Author

I've rebased this onto the current master.
@Aire-One, Could you please review this change?

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.

feat: Add some way for awesome.spawn to set G_SPAWN stdio flags.
4 participants