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

Add "php5-sqlite" to dependencies #1581

Merged
merged 9 commits into from Sep 8, 2017

Conversation

Projects
None yet
6 participants
@WaLLy3K
Copy link
Collaborator

WaLLy3K commented Jul 5, 2017

By submitting this pull request, I confirm the following:

  • I have read and understood the contributors guide.
  • I have checked that another pull request for this purpose does not exist.
  • I have considered, and confirmed that this submission will be valuable to others.
  • I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  • I give this submission freely, and claim no ownership to its content.

How familiar are you with the codebase?:

9


When loading Long Term Data > Query Log, this prevents the following message from popping up:

An unknown error occured while loading the data.

@WaLLy3K WaLLy3K requested a review from DL6ER Jul 5, 2017

@DL6ER
Copy link
Member

DL6ER left a comment

Ubuntu 16.04: php-sqlite3 or php7.0-sqlite3 (consistent across two machines)
Ubuntu 16.10: php-sqlite3 or php7.0-sqlite3 (consistent across three machines)
Ubuntu 17.04: php-sqlite3 or php7.0-sqlite3

Debian 7.11: php5-sqlite
Debian 8.8: php5-sqlite
Debian 9: php-sqlite3 (<-- different than the other Debians)

Raspbian Jessie: php5-sqlite

Fedora 24 & 25: No package available for PHP SQLite support

@WaLLy3K WaLLy3K changed the title Add "php5-sqlite" to dependencies [WIP] Add "php5-sqlite" to dependencies Jul 7, 2017

@WaLLy3K

This comment has been minimized.

Copy link
Collaborator Author

WaLLy3K commented Jul 7, 2017

Given the various names for the package we need (and the potential uncertainty of the package not existing), we might want to consider doing a search for it beforehand with: apt-cache search --names-only "^php" sqlite | cut -d " " -f1

What do you think, @DL6ER?

@DL6ER

This comment has been minimized.

Copy link
Member

DL6ER commented Jul 7, 2017

@WaLLy3K Sounds like a good idea.

@dschaper

This comment has been minimized.

Copy link
Member

dschaper commented Jul 8, 2017

Running on Armbian with php 7 I get the following packages listed:

root@nanopineo:/var/log/lighttpd# apt-cache search --names-only "^php" sqlite | cut -d " " -f1
php-sqlite3
php7.0-sqlite3

The first is php5, the second is php7.

@WaLLy3K

This comment has been minimized.

Copy link
Collaborator Author

WaLLy3K commented Jul 9, 2017

Ah, so we're going to just have to amend the --names-only search to limit it to the users currently installed PHP version. Good catch, @dschaper!

@dschaper

This comment has been minimized.

Copy link
Member

dschaper commented Jul 9, 2017

https://github.com/pi-hole/pi-hole/blob/master/automated%20install/basic-install.sh#L102 This is the check we do during install to see what PHP version is available, would this be of any use for the issue in this thread?

@WaLLy3K

This comment has been minimized.

Copy link
Collaborator Author

WaLLy3K commented Jul 15, 2017

It could work, but I don't know how that'll affect anyone that's wanting to run PHP7 instead of 5.

@WaLLy3K

This comment has been minimized.

Copy link
Collaborator Author

WaLLy3K commented Jul 20, 2017

Currently, it looks like apt-cache search --names-only "^${phpVer}" sqlite | cut -d " " -f1 will be the easiest way to ensure we're getting the right package, since the package name isn't exactly reliable. However, this won't work on the Fedora family. Could anyone confirm what the apt-cache equivalent is?

@PromoFaux

This comment has been minimized.

Copy link
Member

PromoFaux commented Jul 26, 2017

Is there a yum equivalent? @bcambl may know...

@bcambl

This comment has been minimized.

Copy link
Member

bcambl commented Jul 26, 2017

php-pdo is what you are looking for.

PromoFaux added some commits Aug 16, 2017

Add Fedora/Centos dependency
Signed-off-by: Adam Warner <adamw@rner.email>
Work out correct SQLITE version for php-sqlite
Signed-off-by: Adam Warner <adamw@rner.email>
Merge branch 'development' into sqlite-dependency
Signed-off-by: Adam Warner <adamw@rner.email>

# Conflicts:
#	automated install/basic-install.sh

donefixedit

@PromoFaux PromoFaux changed the base branch from development to release/3.2 Aug 16, 2017

@PromoFaux PromoFaux changed the title [WIP] Add "php5-sqlite" to dependencies Add "php5-sqlite" to dependencies Aug 16, 2017

@PromoFaux

This comment has been minimized.

Copy link
Member

PromoFaux commented Aug 16, 2017

Please note the three additional commits are from the development branch. From a PR that probably should have gone into 3.2 in the first place (cosmetic bugs)

@@ -204,7 +209,7 @@ elif command -v rpm &> /dev/null; then
PKG_COUNT="${PKG_MANAGER} check-update | egrep '(.i686|.x86|.noarch|.arm|.src)' | wc -l"
INSTALLER_DEPS=(dialog git iproute net-tools newt procps-ng)
PIHOLE_DEPS=(bc bind-utils cronie curl dnsmasq findutils nmap-ncat sudo unzip wget)
PIHOLE_WEB_DEPS=(lighttpd lighttpd-fastcgi php php-common php-cli)
PIHOLE_WEB_DEPS=(lighttpd lighttpd-fastcgi php php-common php-cli php-pdo)

This comment has been minimized.

@Mcat12

Mcat12 Aug 16, 2017

Member

Is PDO necessary?

This comment has been minimized.

@PromoFaux

PromoFaux Aug 16, 2017

Member

Based off of @bcambl 's comment here:
#1581 (comment)

This comment has been minimized.

@Mcat12

Mcat12 Sep 8, 2017

Member

Verified that (at least on Fedora) it provides the same SQLite library.

@@ -45,7 +45,7 @@ if [ -x "$(command -v rpm)" ]; then
PKG_MANAGER="yum"
fi
PKG_REMOVE="${PKG_MANAGER} remove -y"
PIHOLE_DEPS=( bind-utils bc dnsmasq lighttpd lighttpd-fastcgi php-common git curl unzip wget findutils )
PIHOLE_DEPS=( bind-utils bc dnsmasq lighttpd lighttpd-fastcgi php-common php-sqlite git curl unzip wget findutils )

This comment has been minimized.

@Mcat12

Mcat12 Aug 17, 2017

Member

I think this should be php-pdo?

This comment has been minimized.

@PromoFaux

PromoFaux Aug 17, 2017

Member

Well caught. Uninstall script neglected as-per! One sec...

indent nested function in basic-install.sh
fix dependency in uninstall.sh

Signed-off-by: Adam Warner <adamw@rner.email>

@PromoFaux PromoFaux changed the base branch from release/3.2 to development Aug 28, 2017

@DL6ER

DL6ER approved these changes Sep 8, 2017

@Mcat12

Mcat12 approved these changes Sep 8, 2017

@PromoFaux PromoFaux merged commit 474ac4a into development Sep 8, 2017

5 checks passed

codacy/pr Good work! A positive pull request.
Details
code-review/pullapprove Approved by Mcat12, WaLLy3K
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@PromoFaux PromoFaux deleted the sqlite-dependency branch Sep 8, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.