afrog icon indicating copy to clipboard operation
afrog copied to clipboard

Remove requirement "lsb-core-noarch" from the RPM spec file.

Open expeehaa opened this issue 2 years ago • 4 comments

Requirements for Contributing a Bug Fix

Identify the Bug

See #23560. The dependency lsb-core-noarch is not (properly) supported anymore by many distributions and leads to completely unnecessary and deprecated (e.g. Python 2 on openSUSE Tumbleweed) dependencies. Packages should instead define the minimal requirements they actually have.

Description of the Change

The requirement lsb-core-noarch was removed from the RPM spec file.

Alternate Designs

See #23560 for alternative solutions.

Possible Drawbacks

A dependency of atom may not be required by the RPM anymore.

Verification Process

None, besides an educated guess that the base system requirements are already installed by default. (I could not find documentation on building the package locally, and ./script/build --create-rpm-package failed on installing the script dependencies.)

Release Notes

Dropped the lsb-core-noarch requirement from the RPM.

expeehaa avatar Jun 17 '22 18:06 expeehaa

image

https://github.com/atom/atom/pull/6888

icecream17 avatar Jun 17 '22 19:06 icecream17

Oops, I was wrong. lsb_release is called by the notifications package if there is a bug report popup, apparently.

I should have looked more closely and seen this PR they were talking about: https://github.com/atom/notifications/pull/64

It does appear it has error handling and will proceed with slightly less detailed info if you don't have lsb_release (or if lsb_release errors out for whatever reason).

There are apparently better-supported options than lsb_release on RPM-based distros. For now, either moving it to Suggests: or removing it entirely from the rpm spec seems fair to me. And I think that, long term, the best solution would be to use a different, more-appropriate command on RPM-based distros.

DeeDeeG avatar Jun 23 '22 19:06 DeeDeeG

Oops, I was wrong. lsb_release is called by the notifications package if there is a bug report popup, apparently.

I should have looked more closely and seen this PR they were talking about: atom/notifications#64

It does appear it has error handling and will proceed with slightly less detailed info if you don't have lsb_release (or if lsb_release errors out for whatever reason).

There are apparently better-supported options than lsb_release on RPM-based distros. For now, either moving it to Suggests: or removing it entirely from the rpm spec seems fair to me. And I think that, long term, the best solution would be to use a different, more-appropriate command on RPM-based distros.

There is another (short-term) solution to that. As far as I know, an RPM spec can specify not only required packages, but also required files like this:

Requires: /usr/bin/lsb_release

That would remove the requirement of lsb-core-noarch and at the same time do what should have probably been done in the first place: specify a minimal set of requirements. openSUSE Tumbleweed has a package lsb_release that packages thkukuk/lsb-release_os-release and has /bin/sh as its only requirement while providing only /usr/bin/lsb_release. I wouldn’t be surprised if other RPM-based distros have such a package as well.

If that idea is accepted, I would update the pull request accordingly.

expeehaa avatar Jun 23 '22 20:06 expeehaa

Sounds logical and like a good idea. We've done similar for various shared object (.so) files.

I don't have any permissions to merge things at this repo, but I'll at least comment that I support it. If it's never merged here, I would be interested in doing it at the atom-community/atom fork.

DeeDeeG avatar Jun 23 '22 22:06 DeeDeeG

It's neat that this was merged. Thank you for that @darangi.

Looking closer, I think the author forgot to actually add /usr/bin/lsb_release to the dependencies. Maybe it can be added now? (Or under Recommends: instead,so it won't cause a problem if that program is no-longer available some time in the future?)

I can open a PR to do that if desired.

- DeeDeeG

DeeDeeG avatar Sep 13 '22 16:09 DeeDeeG

@DeeDeeG yes, you can open a PR to address it. I will merge it

darangi avatar Sep 13 '22 16:09 darangi