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

mariadb: initial addon #3190

Merged
merged 1 commit into from Dec 30, 2018

Conversation

@vpeter4
Copy link
Contributor

commented Dec 26, 2018

What should be changed to satisfy LE standards? Or even better: just make changes online :)

PKG_SITE="https://mariadb.org/"
PKG_URL="https://downloads.mariadb.org/interstitial/${PKG_NAME}-${PKG_VERSION}/source/${PKG_NAME}-${PKG_VERSION}.tar.gz"
PKG_DEPENDS_HOST="toolchain ncurses:host"
PKG_DEPENDS_TARGET="toolchain ncurses libaio openssl zlib bzip2 lzo binutils systemd libxml2 mariadb:host"

This comment has been minimized.

Copy link
@CvH

CvH Dec 26, 2018

Member
Suggested change
PKG_DEPENDS_TARGET="toolchain ncurses libaio openssl zlib bzip2 lzo binutils systemd libxml2 mariadb:host"
PKG_DEPENDS_TARGET="toolchain binutils bzip2 libaio libxml2 lzo ncurses openssl systemd zlib mariadb:host"

just ocd

This comment has been minimized.

Copy link
@vpeter4

vpeter4 Dec 27, 2018

Author Contributor

mariadb:host should be last so first host part is build and then target. It looks nicer when checking build log.

This comment has been minimized.

Copy link
@CvH

CvH Dec 27, 2018

Member

makes sense, I changed it

PKG_REV="100"
PKG_SHA256="211655b794c9d5397ba3be6c90737eac02e882f296268299239db47ba328f1b2"
PKG_LICENSE="GPL2"
PKG_SITE="https://mariadb.org/"

This comment has been minimized.

Copy link
@CvH

CvH Dec 26, 2018

Member
Suggested change
PKG_SITE="https://mariadb.org/"
PKG_SITE="https://mariadb.org"
PKG_DEPENDS_HOST="toolchain ncurses:host"
PKG_DEPENDS_TARGET="toolchain ncurses libaio openssl zlib bzip2 lzo binutils systemd libxml2 mariadb:host"
PKG_SHORTDESC="MariaDB is a community-developed fork of the MySQL."
PKG_LONGDESC="${PKG_SHORTDESC}"

This comment has been minimized.

Copy link
@CvH

CvH Dec 26, 2018

Member
Suggested change
PKG_LONGDESC="${PKG_SHORTDESC}"
PKG_LONGDESC="MariaDB ($PKG_VERSION) is a fast SQL database server and a drop-in replacement for MySQL."

hopefully this isn't worse

}

makeinstall_host() {
: #

This comment has been minimized.

Copy link
@CvH

CvH Dec 26, 2018

Member
Suggested change
: #
:

ADDON_ID=service.mariadb
ADDON_DIR="$HOME/.kodi/addons/$ADDON_ID"
ADDON_HOME="$HOME/.kodi/userdata/addon_data/$ADDON_ID"

This comment has been minimized.

Copy link
@CvH

CvH Dec 26, 2018

Member

ADDON_DIR and ADDON_HOME are not needed (yes we are doing it wrong elsewhere too) https://github.com/LibreELEC/LibreELEC.tv/blob/master/packages/mediacenter/kodi/profile.d/00-addons.conf#L18-L20

This comment has been minimized.

Copy link
@vpeter4

vpeter4 Dec 27, 2018

Author Contributor

But you need to call oe_setup_addon() which I don't. But I can add it too.


. /etc/profile

MYSQL_ROOT_PASS=123

This comment has been minimized.

Copy link
@CvH

CvH Dec 26, 2018

Member

could we add this to the kodi settings ? a preset password is always rather meh

This comment has been minimized.

Copy link
@chewitt

chewitt Dec 26, 2018

Member

have a look at the docker add-on, there's an approach for setting the root pass

This comment has been minimized.

Copy link
@vpeter4

vpeter4 Dec 27, 2018

Author Contributor

You mean like this? https://github.com/LibreELEC/LibreELEC.tv/blob/master/packages/addons/service/docker/source/examples/mysql.service#L19-L21
It is still hardcoded (but I will replace 123 with libreelec). User can change password from command line with mysqladmin

This comment has been minimized.

Copy link
@CvH

CvH Dec 27, 2018

Member

what about MYSQL_ROOT_PASS=$MYSQL_PW

then add settings.xml (not tested)

<?xml version="1.0" encoding="utf-8" standalone="yes"?>
<settings>
    <category label="30000">
		<setting label="1010" type="lsep"/>
		<setting type="sep" />
		<setting id="MYSQL_PW" type="text" label="30001" default="" />
    </category>
</settings>

This comment has been minimized.

Copy link
@vpeter4

vpeter4 Dec 27, 2018

Author Contributor

Well, it is used when addon is installed. Which means initial password would always be hardcoded. And when changing it (I think) it is easier to type it on real keyboard than in kodi's virtual one.

This comment has been minimized.

Copy link
@chewitt

chewitt Dec 29, 2018

Member

As a design principle we shouldn't allow static passwords. I'd also advocate we aim for a simple user experience. Not a one-button "for dummies" but reduced complexity through sensible and appropriate GUI controls. Kodi also needs work, but that's a different battle.

My $0.02: Passwords should be stored in the settings.xml for the add-on. If this file pre-exists when the add-on is first installed; read and reuse the root/user password values. Else create a random root password and store it. Provide a simple GUI for setting the credential to use with Kodi. Prompt for username but pre-define "kodi" so that cumbersome on-screen input is not required. Prompt for password but require input. If users choose to set "kodi" as the obvious and easily guessed password it's their mistake not ours.

IP access to the DB is partially covered with the default firewall. This feature currently defaults to OFF but will/should be changed to HOME before 9.0 release. HOME allows connections from RFC1918 networks. It won't protect the small number of idiots who expose their HTPC through deliberate port forwarding, but will protect the larger group of ignorants who expose HTPC services through a VPN connection.

This comment has been minimized.

Copy link
@chewitt

chewitt Dec 29, 2018

Member

NB: Rome wasn't built in a day so perhaps start simple (set a default root user credential only and let users create a user and bad passwords themselves) and add the nicer GUI stuff in a future update.

This comment has been minimized.

Copy link
@vpeter4

vpeter4 Dec 29, 2018

Author Contributor

What about something like that? https://pasteboard.co/HU1hHVg.jpg
When password is changed service is restarted and passwords are enforced with running some init sql commands.
Hardcoded defaults are set to 123/kodi: https://pasteboard.co/HU1jmoV.jpg

This comment has been minimized.

Copy link
@chewitt

chewitt Dec 30, 2018

Member

GUI looks great (pictures are always worth many words!) but I don't see need for hardcoded defaults. If the GUI can handle either password being changed after first-run just create two random passwords for root/kodi and we don't need to do more. Minor nit; the left side Password button should be Passwords as there's more than one. I'll drop into IRC later - ping me if I'm misunderstanding anything.

This comment has been minimized.

Copy link
@chewitt

chewitt Dec 30, 2018

Member

second nit; the usernames should be root/kodi not Root/Kodi

@chewitt

This comment has been minimized.

Copy link
Member

commented Dec 26, 2018

icon_mariadb

^ updated icon to match house style, thanks

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented Dec 27, 2018

@vpeter4 would you mind using this updated package.mk instead? http://ix.io/1ugU

It incorporates the changes suggested by @CvH, but also those that will be required after #3171 - the reference to ${PKG_BUILD} won't be known when package.mk is sourced only once.

@vpeter4 vpeter4 force-pushed the vpeter4:mariadb_addon branch 3 times, most recently from e2fac74 to 0e7116a Dec 27, 2018

@LibreELEC LibreELEC deleted a comment from MilhouseVH Dec 28, 2018

@vpeter4 vpeter4 force-pushed the vpeter4:mariadb_addon branch 6 times, most recently from 2648a26 to 71795f7 Dec 30, 2018

@vpeter4

This comment has been minimized.

Copy link
Contributor Author

commented Dec 30, 2018

Done. Looks like https://pasteboard.co/HU6ZJvG.jpg

@vpeter4 vpeter4 force-pushed the vpeter4:mariadb_addon branch 6 times, most recently from f296156 to 6983daa Dec 30, 2018

@vpeter4 vpeter4 force-pushed the vpeter4:mariadb_addon branch from 6983daa to 81d3bc1 Dec 30, 2018

@chewitt
Copy link
Member

left a comment

LGTM

@CvH CvH merged commit 41d2c65 into LibreELEC:master Dec 30, 2018

@vpeter4 vpeter4 deleted the vpeter4:mariadb_addon branch Dec 30, 2018

@vpeter4

This comment has been minimized.

Copy link
Contributor Author

commented Jan 14, 2019

@MilhouseVH : When function configure_package() must be used and when PKG_CONFIGURE_OPTS_TARGET must be inside of it?

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2019

https://github.com/LibreELEC/LibreELEC.tv/blob/master/packages/readme.md#late-binding-variable-assignment

It's used here because PKG_CMAKE_OPTS_TARGET references ${PKG_BUILD} which is not known until after the package.mk is sourced.

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