Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tidy Up Dependencies #1359

Open
PromoFaux opened this issue Jul 4, 2023 · 10 comments · Fixed by #1366
Open

Tidy Up Dependencies #1359

PromoFaux opened this issue Jul 4, 2023 · 10 comments · Fixed by #1366
Labels

Comments

@PromoFaux
Copy link
Member

PromoFaux commented Jul 4, 2023

Currently, the dockerfile contains:

RUN apk add --no-cache git \
bash \
curl \
bind-tools \
nmap-ncat \
psmisc \
sudo \
unzip \
wget \
libidn \
nettle \
libcap \
openresolv \
iproute2-ss \
jq \
coreutils \
ncurses \
dialog \
procps \
dhcpcd \
openrc \
newt

We need to go through this list and determine what can be removed. It was mostly copied from the existing image

@PromoFaux PromoFaux removed this from Pi-hole v6 Jul 4, 2023
@PromoFaux PromoFaux added v6 never-stale Use this label to ensure the stale action does not close this issue and removed never-stale Use this label to ensure the stale action does not close this issue labels Jul 4, 2023
@PromoFaux PromoFaux moved this to 🏗 In progress in Pi-hole v6 Jul 19, 2023
@PromoFaux
Copy link
Member Author

Pass 1 of this:

#1366

@Gontier-Julien
Copy link
Contributor

@PromoFaux i think for the 2nd pass, we could drop : unzip & wget

For unzip, unless it is used somewhere that i am not aware of (you know the code more than me), it could be dropped.

For wget i think if it is used somewhere we could replace it by curl instead.

For the rest:

bash is currently still required for the bash_functions.sh and other stuff. (we/i could potentially see if it is possible to make it all shell compliant, it would just involve a lot of work and testing tho. Or we could look into it in a near future)

git unless it used else where after the git clones it can be removed before the container is shipped (or with a multistage build)

coreutils could be dropped or moved in the dev-tools since alpine use busybox and should have most of what is needed (https://wiki.alpinelinux.org/wiki/GNU_core_utilities)

@PromoFaux
Copy link
Member Author

Great suggestions, some of them in line with thoughts I'm still trying to order in my head :)

wget / unzip

No problem dropping, unless they are needed by any of the core scripts

bash

In the debian based containers, we've been pretty lazy and just run the install script from the core repo as though it was bare metal. To be fair, it's worked OK doing it like that - but this is a good opportunity to a) bring down the size of the container, b) make it follow "the docker way" more closely (it's never going to be perfect!)

A lot of the scripts in the core repo (https://github.com/pi-hole/pi-hole) are not (yet) posix compliant, so bash is around for as long as we're leaning on the scripts from that repo that require it.

git

I've been thinking about this one a lot recently, and I have a multi stage build locally that's along these lines but it needs more thought. This links in with #1388.

In the existing containers we allow users to run pihole checkout blah blah, which makes testing hotfix branches easier, however the more I think about it the more I think we probably just don't need that functionality in the container. It is nice, but ultimately a lazy option. #1384 aims to make it simple to build the container against custom branches if so desired/required to test.

coreutils

Worth the experment!

@rdwebdesign
Copy link
Member

rdwebdesign commented Jul 19, 2023

git is used by core (pihole -up and pihole updatechecker) to verify if there is an update.
pihole -up is not used on container, but we still check for updates.

@PromoFaux PromoFaux linked a pull request Jul 19, 2023 that will close this issue
1 task
@PromoFaux
Copy link
Member Author

but we still check for updates.

Thinking on it, we just need to get the versions when the container is built, and the only update we should be checking for inside the container (if at all) is the container version. pihole -up has always been disabled inside the container.

@PromoFaux
Copy link
Member Author

Worth the experment!

Need to tweak some of the install calls, that's fine - I just copy pasted them from the installer in the first place, fully expected things to go wrong

------
 > [ 7/11] RUN cd /etc/.pihole &&     install -Dm755 -d /opt/pihole &&     install -Dm755 -t /opt/pihole gravity.sh &&     install -Dm755 -t /opt/pihole ./advanced/Scripts/*.sh &&     install -Dm755 -t /opt/pihole ./automated install/uninstall.sh &&     install -Dm755 -t /opt/pihole ./advanced/Scripts/COL_TABLE &&     install -Dm755 -t /usr/local/bin pihole &&     install -Dm644 ./advanced/bash-completion/pihole /etc/bash_completion.d/pihole &&     install -T -m 0755 ./advanced/Templates/pihole-FTL-prestart.sh /opt/pihole/pihole-FTL-prestart.sh &&     install -T -m 0755 ./advanced/Templates/pihole-FTL-poststop.sh /opt/pihole/pihole-FTL-poststop.sh:
#12 0.434 install: unrecognized option: T
#12 0.437 BusyBox v1.36.1 (2023-06-02 00:42:02 UTC) multi-call binary.
#12 0.437 
#12 0.437 Usage: install [-cdDsp] [-o USER] [-g GRP] [-m MODE] [-t DIR] [SOURCE]... DEST
#12 0.437 
#12 0.437 Copy files and set attributes
#12 0.437 
#12 0.437       -c      Just copy (default)
#12 0.437       -d      Create directories
#12 0.437       -D      Create leading target directories
#12 0.437       -s      Strip symbol table
#12 0.437       -p      Preserve date
#12 0.437       -o USER Set ownership
#12 0.437       -g GRP  Set group ownership
#12 0.437       -m MODE Set permissions
#12 0.437       -t DIR  Install to DIR
------

Easily fixed:

-    install -T -m 0755 ./advanced/Templates/pihole-FTL-prestart.sh /opt/pihole/pihole-FTL-prestart.sh && \
-    install -T -m 0755 ./advanced/Templates/pihole-FTL-poststop.sh /opt/pihole/pihole-FTL-poststop.sh
+    install -Dm755 ./advanced/Templates/pihole-FTL-prestart.sh /opt/pihole/pihole-FTL-prestart.sh && \
+    install -Dm755 ./advanced/Templates/pihole-FTL-poststop.sh /opt/pihole/pihole-FTL-poststop.sh

I'll run it locally for a bit and see if anything else goes bang (not massively worried about focussing here just yet though, total space saving by removing wget , unzip, and coreutils (I'm not ready for git yet) is a whopping 2MB 😅

On a more extreme branch I'm down to 99.5MB, but I've not tested anything in that yet. More of a "because I can" thing

@Gontier-Julien
Copy link
Contributor

Yup agreed with what y'all said, about git (i've came across it when i was experimenting to reduce the size further, going to make the pull request after that)

@PromoFaux
Copy link
Member Author

PromoFaux commented Jul 20, 2023

Removing ncurses broke the pihole -v output. I will add it back in for the time being until we decide the direction we will be going with the version checker/multistage build etc

pihole:/# pihole -v
/opt/pihole/COL_TABLE: line 2: tput: command not found
  Pi-hole version is development-v6 eda83a4 (Latest: v5.17.1)
  AdminLTE version is development-v6 395ceef (Latest: v5.20.1)
  FTL version is development-v6 vDev-5a6d86a (Latest: v5.23)
pihole:/# apk add ncurses
fetch https://dl-cdn.alpinelinux.org/alpine/v3.18/main/x86_64/APKINDEX.tar.gz
fetch https://dl-cdn.alpinelinux.org/alpine/v3.18/community/x86_64/APKINDEX.tar.gz
(1/1) Installing ncurses (6.4_p20230506-r0)
Executing busybox-1.36.1-r0.trigger
OK: 40 MiB in 66 packages
pihole:/# pihole -v
  Pi-hole version is development-v6 eda83a4 (Latest: v5.17.1)
  AdminLTE version is development-v6 395ceef (Latest: v5.20.1)
  FTL version is development-v6 vDev-5a6d86a (Latest: v5.23)

@rdwebdesign
Copy link
Member

wget / unzip

No problem dropping, unless they are needed by any of the core scripts

I think unzip is not necessary as dependency.

It is used to decompress teleporter files, but FTL now has an embedded zip:
https://github.com/pi-hole/FTL/tree/development-v6/src/zip.

@PromoFaux
Copy link
Member Author

I think unzip is not necessary as dependency.

Cool, feel free to try it and PR if it works :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: 🏗 In progress
Development

Successfully merging a pull request may close this issue.

3 participants