chromebrew icon indicating copy to clipboard operation
chromebrew copied to clipboard

Remove need for essential_deps field in device.json

Open Zopolis4 opened this issue 1 year ago • 18 comments

Description

The definition of what is an 'essential' package has been confused. Essential packages are those that are absolutely required for chromebrew to run, and only chromebrew. If you remove lz4, zstd can no longer decompress .tar.lz binaries-- this is not required for ruby to run, and thus is not an essential package.

As such, the list of essential packages is actually quite short, and the only complexity required is in ensuring we have the right glibc_lib_* or glibc_build_* package.

Tested & Working properly:

  • [x] x86_64
  • [x] i686
  • [x] armv7l

Run the following to get this pull request's changes locally for testing.

CREW_REPO=https://github.com/Zopolis4/chromebrew.git CREW_BRANCH=ughhhh crew update \
&& yes | crew upgrade

Zopolis4 avatar Sep 01 '24 22:09 Zopolis4

Also, we want to make sure zstd isn't removed, because otherwise we are going to get support issues with people who didn't realize they were breaking their systems.

satmandu avatar Sep 01 '24 22:09 satmandu

Being able to install packages requires zstd and its dependencies, so I don't understand why zstd isn't essential

zstd is essential, which is why I've kept it as an essential package.

Zopolis4 avatar Sep 01 '24 22:09 Zopolis4

image as you can see, removing lz4 does not break the basic functionality of crew, hence why it is not an essential package.

Zopolis4 avatar Sep 01 '24 22:09 Zopolis4

chronos@cheekon-x86_64:2.27 M90 /usr/local/lib/crew/packages (master >|SPARSE)$ upx -d `which zstd`
                       Ultimate Packer for eXecutables
                          Copyright (C) 1996 - 2024
UPX 4.2.4       Markus Oberhumer, Laszlo Molnar & John Reiser    May 9th 2024

        File size         Ratio      Format      Name
   --------------------   ------   -----------   -----------
   1102781 <-    382512   34.69%   linux/amd64   zstd

Unpacked 1 file.
chronos@cheekon-x86_64:2.27 M90 /usr/local/lib/crew/packages (master >|SPARSE)$ ../tools/getrealdeps.rb zstd
Checking for the runtime dependencies of zstd...

Package zstd has runtime library dependencies on these packages:
  depends_on 'gcc_lib' # R
  depends_on 'glibc' # R
  depends_on 'lz4' # R
  depends_on 'xzutils' # R
  depends_on 'zlib' # R
chronos@cheekon-x86_64:2.27 M90 /usr/local/lib/crew/packages (master >|SPARSE)$ lddtree `which zstd`
/usr/local/bin/zstd (interpreter => /lib64/ld-linux-x86-64.so.2)
    libz.so.1 => /usr/local/lib64/libz.so.1
    liblzma.so.5 => /usr/local/lib64/liblzma.so.5
    liblz4.so.1 => /usr/local/lib64/liblz4.so.1
    libpthread.so.0 => /usr/local/lib64/libpthread.so.0
    libc.so.6 => /usr/local/lib64/libc.so.6
    ld-linux-x86-64.so.2 => /lib64/ld-linux-x86-64.so.2
chronos@cheekon-x86_64:2.27 M90 /usr/local/lib/crew/packages (master >|SPARSE)$

satmandu avatar Sep 01 '24 22:09 satmandu

All dependencies of zstd need to be installed...

satmandu avatar Sep 01 '24 22:09 satmandu

Otherwise installs break.

satmandu avatar Sep 01 '24 23:09 satmandu

All dependencies of zstd need to be installed...

This is not the case. As I have shown, lz4 can be removed and crew will still function.

Zopolis4 avatar Sep 01 '24 23:09 Zopolis4

All dependencies of zstd need to be installed...

This is not the case. As I have shown, lz4 can be removed and crew will still function.

My experience is that installs break.

Please confirm that installs do not break with your changes by passing the REPO and BRANCH flags to the installer after deleting Chromebrew in a container.

We should really just have a unit test for this triggered with any changes in install.sh or to stuff like packages in buildessential and core when packages in those have their dependencies change, just to make sure installs don't break. Would that be overkill?

(Maybe a locally triggered unit test that doesn't burden GitHub's Action servers would make the most sense? Doing a test install takes about 15 min on my systems at most.)

satmandu avatar Sep 01 '24 23:09 satmandu

All dependencies of zstd need to be installed...

This is not the case. As I have shown, lz4 can be removed and crew will still function.

Yes, but do fresh installs still work with it removed? We cannot break fresh installs under any circumstances or we will antagonize new users and create a stampede of incoming issues.

uberhacker avatar Sep 01 '24 23:09 uberhacker

(I'm not trying to be dogmatic for the sake of being dogmatic — just being overly protective because it is annoying to track down breakage after the fact. It would be nice to delegate that protectiveness to some automatic mechanism like we have done with our other unit tests, because refactoring can be really nice to streamline and cut out excess.)

satmandu avatar Sep 01 '24 23:09 satmandu

I have just tested a fresh install using this branch, everything works.

Zopolis4 avatar Sep 01 '24 23:09 Zopolis4

I have just tested a fresh install using this branch, everything works.

Just keep in mind that dependencies are lowest common denominator for all of our systems for the most part.

Do installs work on i686 + pre-M126/post-M126 systems on both x86_64 + Arm...

There is a reason that essential packages is currently set to expand to include all dependencies.

If zstd doesn't have dependencies installed, it will try to use system libraries. If those aren't available... I think crew then tries to use the system zstd... That isn't available on the oldest systems.

satmandu avatar Sep 01 '24 23:09 satmandu

What i'm trying to convey here is that zstd doesn't need some dependencies in order to function for chromebrew's usage of it. zstd is able to decompress .tar.zst files and run ruby without lz4 installed, so we don't need lz4 for basic functionality.

Zopolis4 avatar Sep 01 '24 23:09 Zopolis4

i.e. we aren't breaking zstd by removing lz4. we might be removing its ability to decompress .tar.lz files, but it still works for ruby's purposes.

Zopolis4 avatar Sep 01 '24 23:09 Zopolis4

Also, I expanded essential deps because installs were failing...

satmandu avatar Sep 02 '24 00:09 satmandu

Also, I expanded essential deps because installs were failing...

In what situations were they failing? If I can test this PR and it works in that case, then we should be pretty good.

Zopolis4 avatar Sep 02 '24 00:09 Zopolis4

any update on this? I'd like to get this PR in before it joins the pile of "out of sight out of mind" pull requests that have been waiting on reviews for months

and i've got some cleanup changes after this, including dealing with the warning constant message on crew startup, so i'd like to get those in as well

Zopolis4 avatar Sep 09 '24 03:09 Zopolis4

Will look at this again after I finish this massive python cleanup/update I'm on final approach for...

satmandu avatar Sep 09 '24 11:09 satmandu

  1. What was the reason to remove generate_compatible? I was using that to refresh crew's state of updatable packages locally without doing a git pull.
  2. Do you want to solve https://github.com/chromebrew/chromebrew/issues/10981 with this work? It will require you to test in the M124 container:
#!/bin/bash
# crewbase-octopus-x86_64.m124.sh
docker pull satmandu/crewbase:octopus-x86_64.m124
docker run --rm -v $(pwd):/output -h $(hostname)-x86_64 -it satmandu/crewbase:octopus-x86_64.m124 /bin/bash

You can do the usual dance to test the install in the container:

passwd -d chronos
echo "chronos ALL=(ALL) NOPASSWD: ALL" >> /etc/sudoers
chown chronos /usr/local
/usr/bin/sudo -E -n -u chronos /bin/bash
cd /tmp
export HOME=/home/chronos/user/
curl -OLf https://github.com/Zopolis4/chromebrew/raw/refs/heads/ughhhh/install.sh
chmod +x install.sh
cd /usr/local
OWNER=Zopolis4 REPO=chromebrew BRANCH=ughhhh /tmp/install.sh

satmandu avatar Jan 07 '25 15:01 satmandu

Re: generate_compatible-- I don't see anything in that code that does anything other than check essential deps and package compatibility-- what exactly was your usecase?

I'll check out that other bug soon, swapped to a new laptop and distro and so am still setting up my development environment.

Zopolis4 avatar Jan 10 '25 12:01 Zopolis4

Unit tests appear to be hanging-- I'll look into this when I get back.

Zopolis4 avatar Feb 07 '25 07:02 Zopolis4

It looks like removing packages previously deemed essential, such as gcc_lib, is breaking the container unit tests that check to ensure that trying to remove essential packages doesn't break things.

satmandu avatar Feb 07 '25 13:02 satmandu

FIxed-- the issue was just about using yes | ... instead of crew -f in the unit tests.

It looks like removing packages previously deemed essential, such as gcc_lib, is breaking the container unit tests that check to ensure that trying to remove essential packages doesn't break things.

gcc_lib is still listed as an essential package and cannot be removed.

Zopolis4 avatar Feb 08 '25 00:02 Zopolis4