simplify rpm package #4743
simplify rpm package #4743
Conversation
| @@ -227,7 +231,6 @@ const config = { | |||
| ], | |||
| afterInstall: distAssets('linux/after-install.sh'), | |||
| afterRemove: distAssets('linux/after-remove.sh'), | |||
| depends: ['libXScrnSaver', 'libnotify', 'dbus-libs'], | |||
There was a problem hiding this comment.
This is not an improvement for Fedora users, but rather a problem. If the dependencies are not specified a Fedora user can install the app, but it might now work properly. That will generate a lot of frustrated emails to our support.
There was a problem hiding this comment.
@faern I think this concern is opinionated and unfounded:
- This is already the standard with the deb package
- The needed dependencies are popular and likely already installed for most desktop environments
- Fedora isn't the only rpm distro around and it's users should not be given special treatment
There was a problem hiding this comment.
The needed dependencies are popular and likely already installed for most desktop environments
That's not how dependencies work. A package should specify what it needs to run. Not what it needs to run that is likely not installed.
Fedora isn't the only rpm distro around and it's users should not be given special treatment
Just because someone produce an RPM does not mean they intend to support every platform using that format. We currently have the capacity to support Fedora, and we made the decision to support Fedora. That does not mean we have the capacity to do QA for every RPM based distro.
There was a problem hiding this comment.
@faern I've been packaging things for over a decade now and fully understand how dependencies work, but the reality is that your team has tightly coupled your build process to electron and thus are locked into certain limitations - this is one of them.
Just because someone produce an RPM does not mean they intend to support every platform using that format. We currently have the capacity to support Fedora, and we made the decision to support Fedora. That does not mean we have the capacity to do QA for every RPM based distro.
This seems really arbitrary. So instead of removing some dependencies that you aren't even able to justify the reasons they exist, as demonstrated by their absence in debian, despite them still being needed, and instead choose to alienate a bunch of potential or existing customers?
This seems like a stupid hill to die on.
Edit 2: I dug into this general topic a little further and found that there are other problematic dependencies that would require per-distro specification that mullvad has accepted is beyond the purview of these packages. Given that this is already an accepted reality, I would ask that you reconsider. Updating the wiki would cover more distros, as well as incetivize community members from the respective distros to work towards their own native package like everything else. And the only way to get that much community interest is to get the product in the hands of people in the first place.
There was a problem hiding this comment.
as demonstrated by their absence in debian
The reason they are not in the .deb is probably because it has never been an issue. Both .rpm and .deb started out without many dependencies at all. Then we added them as we found out it caused issues. So the only reason we added it to the rpm is likely because users hit issues, but Debian/Ubuntu users did not. A very pragmatic approach.
However. This was added in 2018 and probably not revised since then. I see now that a minimal install of Fedora 38 comes with dbus-libs installed. So it's as you say, very unlikely that any recent Fedora install does not have this library. It was probably different in 2018.
I'm still not sure about removing libXScrnSaver and libnotify. I will need to discuss with @raksooo about those. But are those also problematic to OpenSUSE or is it just dbus-libs?
I dug into this general topic a little further and found that there are other problematic dependencies that would require per-distro specification that mullvad has accepted is beyond the purview of these packages. Given that this is already an accepted reality
That's a different situation. Tray icons on Linux is a terrible ecosystem right now. And there are multiple ways of doing it. So we don't force any solution on the user since they might want another solution. With dbus-libs there is no alternative (for Fedora) so we simply installed the one required package.
There was a problem hiding this comment.
@faern Thanks for taking a deeper looking this. I appreciate it.
are those also problematic to OpenSUSE or is it just dbus-libs?
AFAIK, it's only dbus-libs.
My personal take would be remove all dependencies and instead document them, that way the approach to dependency resolution is always the same - read the docs.
If I really wanted to figure this out, I'd probably try to to get the build system to create per-distro packages using a map of distro dependencies, but that also seems like a lot of work to maintain for a small team.
There was a problem hiding this comment.
People don't read docs. We'll get flooded by emails from users who fail to start the app. I'm OK with accepting this PR if we only remove dbus-libs
If I really wanted to figure this out, I'd probably try to to get the build system to create per-distro packages using a map of distro dependencies, but that also seems like a lot of work to maintain for a small team.
If we can avoid it we don't want more artifacts. Because it blows up almost exponentially (two architectures etc) and we need to redesign the download page to have more download buttons etc. It all just escalates.
There was a problem hiding this comment.
as a Fedora user I agree. Your instructions are already a few commands, just include the command to install the needed packages and you are done.
There was a problem hiding this comment.
Not everyone just copy paste the commands. Typing them out becomes harder. And especially harder to memorize, for those not looking at our instructions every time. It becomes less intuitive to install the app.
If users explicitly install these dependencies, then they won't be marked as dependencies. So if you in the future uninstall the Mullvad app, the dependencies won't get removed, because they are marked as user installed. This is not ideal.
Do you have examples of other software that knowingly omit actual hard dependencies, just to make a single rpm compatible with multiple distros? That sounds like a smelly antipattern to me, without being an RPM packaging expert at all.
cornfeedhobo
force-pushed
the
remove-rpm-deps
branch
from
ecf5e35 to
82704c4
Compare
This commit simplifies the RPM package to not include dependency declarations that are likely to be different across distributions.
cornfeedhobo
force-pushed
the
remove-rpm-deps
branch
from
82704c4 to
3f4f557
Compare
| extraResources: [ | ||
| { from: distAssets(path.join(getLinuxTargetSubdir(), 'mullvad-problem-report')), to: '.' }, | ||
| { from: distAssets(path.join(getLinuxTargetSubdir(), 'mullvad-setup')), to: '.' }, | ||
| { from: distAssets(path.join(getLinuxTargetSubdir(), 'libtalpid_openvpn_plugin.so')), to: '.' }, | ||
| { from: distAssets(path.join('binaries', '${env.TARGET_TRIPLE}', 'openvpn')), to: '.' }, |
| extraFiles: [{ from: distAssets('linux/mullvad-gui-launcher.sh'), to: '.' }], | ||
| extraFiles: [ | ||
| { from: distAssets('linux/mullvad-gui-launcher.sh'), to: '.' }, | ||
| { from: distAssets(path.join(getLinuxTargetSubdir(), 'mullvad-daemon')), to: '.' }, |
There was a problem hiding this comment.
Won't this install mullvad-daemon in two places? Further down we install it to /usr/bin/ also.
|
I'm closing this for two reasons. The first one is that it's stale. There are questions and feedback that has not been processed. And secondly we do not want to make it harder to install the app on our supported distros. We do not support OpenSUSE. |
This commit simplifies the RPM package to not include dependency
declarations that are likely to be different across distributions.
Fixes #2242
This change is