-
Notifications
You must be signed in to change notification settings - Fork 17.3k
Fix RPM dependencies #22076
Fix RPM dependencies #22076
Conversation
|
@DeeDeeG Could you please test if this works for you on a bare RPM-based virtual machine? |
|
@DeeDeeG did you already find some time to test these dependencies on your VM? |
|
Hi @mfonville. I am not totally sure where to get a "minimal" Fedora environment that can also run the GUI app and confirm it really works. I can build this branch and see if it will install on Fedora Workstation (https://getfedora.org). That has a fair amount of stuff already on it. But it would be some sort of validation, at least. |
|
Here's what I get when I try to install the built
Edit to add: If you could give some insight how you found these, it would satisfy my curiosity, and hey maybe it would give me a hint of how better to test the end result. But anyway, I hope that info is helpful. |
|
Ah, okay. I see that So from reading that, I believe the logical |
|
Thanks @DeeDeeG I will add the parentheses. I determined the libraries by compiling atom on an a very bare install, and closely tracked which libraries where required in the linking process (adding them one by one). With respect to the |
|
I now get a build error with |
|
Apparently Any newer version of Ubuntu than Xenial should fix this. Also I'd like to point out that Xenial lost support two weeks ago (April 2021, see https://wiki.ubuntu.com/Releases ) @sadick254 @darangi can we please upgrade the Ubuntu LTS we are building Atom on from Xenial to either Bionic or Focal? |
|
I think it could be advantageous to see what it takes to get newer When you build on newer Ubuntu, you build C code against newer Stuff built for older So the older the OS we build on, the broader compatibility. (And the older our compiler, so yeah it does probably slightly improve performance to build on newer OSes... There is a tradeoff. At least we are only one or two major versions behind for GCC/Clang.) I think a couple of weeks might be a bit early, since a few stragglers might still be on Xenial. There is also the matter that different distros stabilize at different times (and lock in different Though, theoretically, having a cut-off date where we move over to Bionic might be a good idea, so as to not put it off forever. My two cents. |
|
@DeeDeeG I have posted my response on that consideration here: #22416 (comment) If there is another method of installing an updated Does Atom btw have any telemetry data on which platform versions of the users? Because I wonder how many users of Atom would be on releases that are older than the latest stable/LTS release. |
If this documentation is right, it looks like Atom's telemetry (only) includes OS info via the Chrome User Agent string. Unfortunately that is not very fine-grained about what Linux you're using. Here's a Chromium UA string from Xenial in VirtualBox:
The bit that identifies the OS/system is just It is more useful on macOS and Windows, I believe. Chrome on macOS gives you the major_minor_patch version of the OS. Chrome on Windows apparently gives the NT version which you can translate to Windows 10, 8, 7, and so on. |
Here is the list of URLs to the .debs that we would need, in order to get newer Dependency tree (click to expand)We could fetch these and install them before the build. (These are the set of Quick shell script to download and install these... (click to expand)#!/bin/bash
mkdir bionic_rpm_debs && cd bionic_rpm_debs
for deb_URL in `cat path/to/deb_URLs.txt`
do
echo "Downloading ${deb_URL}..."
curl -L "${deb_URL}" -O
done
sudo apt install -y ./*.deb
cd ..I'm looking into the files of them to check the lincenses; So far all are GPL, and their file sizes are small, so we could probably check them in to this repo. That secures against deprecation/archiving of the Bionic repos (at which point these URLs might go down). Edit: I opened a PR targeting your branch that does this. See: mfonville#2 |
|
I get this message after installing the
Looks like More missing:
These are a bunch of X11 shared objects/libraries. Was there a package that used to pull these all in for us? I think an overall X11 package should do to pull all these in, if possible.. More:
Then with all of those added, it worked. (Docker images are super super minimal. I wouldn't guess that most of this is realistically likely to be missing on a real desktop OS.) |
|
Thanks for the input @DeeDeeG When I was figuring out the dependencies I was doing it through Ubuntu/Debian, and these libraries were then pulled in by other 'higher' dependencies that I declared. |
|
I think we can solve it by requiring
I will try to apply these changes and update this PR update: not in all distros the name update2: also we can drop |
|
@DeeDeeG can you test my latest changes? |
Fedora ✅Nice. Testing on the latest Fedora Docker image again. This installs, and I can do Note: I even installed the old one + all the extra libs it needed, then reinstalled this over it, and it didn't add any new package requirements. So this updated list of dependencies is still pretty lean. 👍 openSUSE Leap ❌I get this:
Although if I press Note: Now that I have thought of a way of testing openSUSE (that is: in Docker), we can properly test where package names might be usable on openSUSE, rather than shared object filenames. But obviously the |
|
I had been checking the support for openSUSE too when I made my changes :-) I had not touched the PS: when making my PR I checked all my changes at https://pkgs.org/ to confirm compatibility with OpenSUSE, Fedora and Red Hat. That is why I had to work with |
|
@DeeDeeG Did you test the latest version at your OpenSUSE? |
|
Oops, yes I think it worked. I can try again. (Juggling too many things, I think.) |
|
If you could verify to be sure, please :-) |
|
Wider testing this time. I didn't expect it would make a difference, but it actually did.
Something should be done about It should be possible to include these dependencies as package names if you want, considering the following details (although I leave it to your judgment):
|
|
Pfff, they make it quite a puzzle.
Update: We don't even specify Update2: Neither do we specify Update3: |
Require gtk3 and libgbm(library), drop their lower dependencies
|
I pushed now a rebase on master, that has the updated rpm from bionic, to see if it passes. @DeeDeeG maybe you can retry with the new artifacts that will be published by the CI. |
I can install the libraries individually with I believe these need to be added to the spec file explicitly, or they will be missing... in older Fedora/newer openSUSE Docker images, anyway. Maybe it's time to call it "good enough"? Recall that Atom requires a graphical desktop to be meaningfully usable. And as far as I know any graphical desktop will already have these last bits installed. That was likely true of most, if not all of these basic libraries we have been trying to include for the past several comments. The alternative I think is to add them to the spec file.
I see that CI runs for PRs do not publish |
Ah, then I first misunderstood. It will add them to the spec (they are also specified for Debian, so it is logical that they are required in this spec too) |
|
Here are my testing results and a description of the test method. Conclusion: looks good to me. Results:
Test info:
dnf install -y curl unzip || zypper install -y curl unzip # dnf == package manager in Fedora, zypper == package manager in openSUSE
curl -L 'https://dev.azure.com/DeeDeeG/7690c340-f92e-4cfc-838c-5f2c3651b584/_apis/build/builds/1122/artifacts?artifactName=atom.x86_64.rpm&api-version=6.0&%24format=zip' -o _atom.zip
unzip _atom.zip
dnf install -y ./atom.x86_64.rpm/atom.x86_64.rpm || zypper install -y --allow-unsigned-rpm ./atom.x86_64.rpm/atom.x86_64.rpm
atom-dev --version --no-sandbox
/usr/share/atom-dev/atom --version --no-sandbox
apm-dev --version
It passes if the # atom-dev --version --no-sandbox
Atom : 1.59.0-dev-f55dcdae
Electron: 9.4.4
Chrome : 83.0.4103.122
Node : 12.14.1
# apm-dev --version
apm 2.6.2
npm 6.14.13
node 12.14.1 x64
atom 1.59.0-dev-f55dcdae
python
git 2.31.1Note: Testing Footnotes:[1] On openSUSE Leap 15.3 development preview, the repos are slightly broken, but I do not think this is related to Atom whatsoever. (To install Atom: Run [2] Checking if |
DeeDeeG
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me (installs properly in Fedora and openSUSE, without any missing dependencies as far as I can tell).
|
Thanks for all the testing @DeeDeeG I am happy that all dependencies are now all good. @sadick254 could you merge both this PR and #22015 ? Thank you! |
sadick254
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @mfonville and @DeeDeeG for this. 👍
Addendum to PR #22015 to also fix the dependencies for RPM packages.
Some dependencies could be done through package names, but some had to be specified by its libraries because Fedora and OpenSuse etc don't stick to the same packagenames.
Does need testing/confirmation that it is correct.