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

dbus: rename "enableSystemd" to "withSystemd" #234463

Closed
wants to merge 1 commit into from

Conversation

KAction
Copy link
Contributor

@KAction KAction commented May 27, 2023

Naming convention Support seems to be more popular than enable in
general, in case of systemd -- 59 to 34 as far as "git grep" concerned, so it
makes sense to unify flags under already dominant convention.

Merging such changes treewide is hard (ref: #177832), so here I follow
suggestion of @doronbehar and make one pull request per package affected.

Naturally, this is no-rebuild change.

@KAction
Copy link
Contributor Author

KAction commented Jun 2, 2023

Pinging the only member of teams.freedesktop: @jtojnar

@jtojnar
Copy link
Contributor

jtojnar commented Jun 2, 2023

I like this in principle but I am not sure which direction is the proper one.

Naming convention Support seems to be more popular than enable in
general, in case of systemd -- 59 to 34 as far as "git grep" concerned, so it
makes sense to unify flags under already dominant convention.

What about other flags? Just make sure enable* arguments are not more common overall and moving in this direction does not cause more disunity.

@KAction
Copy link
Contributor Author

KAction commented Jun 3, 2023

I think we should start with unifying on per-feature basis, because different features have different dominant convention. When we finish that part, overlay would be able to enableFoo = true; supportBar = false. I guess after that is done, we will apply the same reasoning -- what is more popular over features.

Support is preferred in some:

>> git grep enableX11|wc -l
68
>> git grep x11Support|wc -l
183
>> git grep enableDbus|wc -l
13
>> git grep dbusSupport|wc -l
31

but not in other:

>> git grep enableQt|wc -l
79
>> git grep qtSupport|wc -l
2

And some have no dominant convention at all (which will be probably the hardest part).

>> ./aux/popularity-contest.sh sdl
support sdl: 16
with sdl: 18
enable sdl: 17

Personally, I don't like support convention at all. Ideally, we would follow conventions of autoconf, where enable means optional feature that does not imply addition external dependency, and with means external dependency. But we are nowhere close to think about such things.

@KAction
Copy link
Contributor Author

KAction commented Jun 3, 2023

Oh, wait.

>> git grep -i withsystemd|wc -l
79

Naming convention <foo>Support seems to be more popular than enable<Foo> in
general, in case of systemd -- 59 to 34 as far as "git grep" concerned, so it
makes sense to unify flags under already dominant convention.

Merging such changes treewide is hard (ref: NixOS#177832), so here I follow
suggestion of @doronbehar and make one pull request per package affected.

Naturally, this is no-rebuild change.
@KAction
Copy link
Contributor Author

KAction commented Jun 3, 2023

UPD: I changed this pull request to withSystemd because of:

>> ./popularity-contest.sh systemd
support systemd: 64
with systemd: 84
enable systemd: 30

I still maintain that we are better off to start on per-feature basis.

@KAction KAction changed the title dbus: rename "enableSystemd" to "systemdSupport" dbus: rename "enableSystemd" to "withSystemd" Jun 3, 2023
@KAction
Copy link
Contributor Author

KAction commented Jun 3, 2023

@jtojnar

>> cat popularity-contest.sh
#! /bin/sh -eu
feature=$1
echo -n "support $feature: "
git grep -i "$1support" | wc -l
echo -n "with $feature: "
git grep -i "with$1" | wc -l
echo -n "enable $feature: "
git grep -i "enable$1" | wc -l

@ghost
Copy link

ghost commented Jun 8, 2023

What about other flags? Just make sure enable* arguments are not more common overall and moving in this direction does not cause more disunity.

👍

Frankly the situation is a bit of a mess though. Someday somebody should write the world's shortest RFC, suffer through a ton of bikeshedding, and get people to pick one or the other.

@KAction
Copy link
Contributor Author

KAction commented Jun 9, 2023

Yes, it is mess. And my experience with trying to put stuff into policy before actually implementing that policy is non-satisfactory (#175650). Instead, I find it much much more effective to actually do things first (e.g #208962).

So let me suggest again that we keep the scope of the project small for now -- just make systemd flags uniform.

@mohe2015
Copy link
Contributor

I think it would be better to create an RFC and then slowly move everything to the same format. Because if we do this here now to my understanding everyone that overrides a package needs to then find out which one they need to change to and then potentially change again if we would unify it over whole nixpkgs later. Also if we slowly unify to one directly you would instantly know how you need to update your code because there is only one convention then.

@KAction
Copy link
Contributor Author

KAction commented Dec 7, 2023

Yeah, I'll draft the RFC. Eventually. Closing this issue.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/pre-rfc-standardization-of-feature-parameters/38104/1

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/pre-rfc-standardization-of-feature-parameters/38104/47

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/pre-rfc-standardization-of-feature-parameters/38104/59

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/pre-rfc-standardization-of-feature-parameters/38104/69

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants