ng0 writes: > Hi Chris, > > thanks for your review. I'll have no time to address it before next week, > just a couple of comments. No worries! Whenever you can update it, that's great! We're almost there; I think the next update will probably be the final one. > Chris Marusich transcribed 9.7K bytes: >> >> ... >> >> ng0 writes: >> >> > I've changed my mind. As there's also 'i3lock' (without the -color suffix) >> > but we don't package it (yet), it would take away the freedom to >> > decide which i3lock you want to use. >> > The author of i3lock-fancy wantsus to use i3lock-color, but there's >> > also a check and it falls back to the normal i3lock. >> > >> > Therefore I think it must be up to people using i3lock-fancy to >> > install i3lock-color AND i3lock-fancy in their system profile >> > (or just i3lock-color in systemprofile + suid it, and i3lock-fancy >> > in the user profile). >> >> I agree with your assessment. For now, I think that's a fine plan, >> although I dislike the fact that by adding the i3lock-fancy package by >> itself, we will make it possible for someone to naively install just >> that package to their profile, only to find that it doesn't work because >> i3lock itself is not installed in the system as a screen locker program. >> However, I can't think of a better solution at this time, and we're >> already doing something similar for the other screen locker packages. >> For example, if you install xscreensaver into your profile without also >> installing it into the system as a screen locker service, xscreensaver >> won't work because it won't be setuid-root. So I think it's OK. > > We'll continue to have these problems with applications, maybe we should > consider it a bug and find a common description. So far it's limited to > screensavers, but I can't think of a reason why this problem would stay > limited to applications of the screensaver class. I've created an email thread to discuss this in more detail on guix-devel. The subject is: "Installing some packages results in "incomplete deployment"". Please look for it and reply there if you have any other thoughts on that matter. For this patch, though, I think it's fine to ask users to install i3lock-fancy and also add to their operating system declaration a screen locker service that uses i3lock. That's how the existing screen lockers have been packaged. > For this specific application (i3lock-fancy), I could ask meskarune if she'd like to add > an error message if none is place already. At the top of the script > check for the existence of the i3lock binary in the users path (simply > 'i3lock', not (only) total paths). If not found, throw an error and abort > with a message that "you need to install i3lock-color". I think that would be nice. I also think we can add i3lock-fancy to Guix even before she makes those changes. >> > i3lock-color should be removed before commiting this patch, >> > and we should add a note about this to the description. Like: >> > "You will need to install i3lock or one of its variants (like >> > i3lock-color) to make use of i3lock-fancy." >> >> I don't understand: why do we need to remove i3lock-color? > > i3lock-fancy is Bash script. I'm patching in the references it > should keep. The 2 calls of i3lock in there must be just 'i3lock', > this will call the binary with the suid, not for example the one > in the store path of i3lock-color (which never worked). > More correct would be this remark: > i3lock-color should be removed from the inputs before commiting > this patch, and we should add a note about this to the description. I understand now, and I agree. -- Chris