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

net-analyzer/graphite-web: bump to 1.1.3, supports Python 3 #8962

Closed
wants to merge 2 commits into from

Conversation

sbraz
Copy link
Member

@sbraz sbraz commented Jun 24, 2018

Hi @grobian I didn't see that you had bumped graphite-web. I think you could pick some of my changes.

  • Do not require twisted or txAMQP, they are not directly used by graphite-web.
  • Simplify dependencies for django.
  • Require cairocffi instead of pycairo.
  • Do not require whisper. It is optional and in the future we could probably use carbon with ceres instead of whisper.
  • Remove data_files and scripts with sed instead of a patch.
  • Always use setuptools, never distutils.
  • pytz and pyparsing are not bundled any more, drop the patch.
  • The manage app doesn't exist any more, fix pkg_config() accordingly.
  • The previously missing share.png is now present in the tarball, remove it from SRC_URI.
  • Run build-index during pkg_config() instead of just creating an empty file. This allows us to drop the complex python file parsing.
  • Mention build-index in the postinst message.
  • Only display the postinst message for new installs.
  • Create the /var/{lib,log}/graphite-web directories which are referenced in local_settings.py.

@gentoo-bot
Copy link

Pull Request assignment

Areas affected: ebuilds
Packages affected: net-analyzer/graphite-web

net-analyzer/graphite-web: @grobian

No bugs to link found. If your pull request references any of the Gentoo bug reports, please add appropriate GLEP 66 tags to the commit message and ping us to reset the assignment.

If you do not receive any reply to this pull request, please open or link a bug to attract the attention of maintainers.

In order to force reassignment and/or bug reference scan, please append [please reassign] to the pull request title.

@gentoo-bot gentoo-bot added assigned PR successfully assigned to the package maintainer(s). no bug found No Bug/Closes found in the commits. labels Jun 24, 2018
@sbraz sbraz added the do not merge Please DO NOT MERGE this PR. It will not be assigned but it will be scanned by CI. label Jun 24, 2018
@grobian
Copy link
Member

grobian commented Jun 24, 2018

I'm a bit confused, many of the thing you mentioned, I already tackled. What's the differences between the current in-tree ebuild and yours?

@sbraz
Copy link
Member Author

sbraz commented Jun 24, 2018

I didn't notice you had bumped it before submitting the PR here. I was waiting for python-ldap to get Python 3 support. These are still not fixed:

  • Do not require twisted or txAMQP, they are not directly used by graphite-web.
  • Do not require whisper. It is optional and in the future we could
    probably use carbon with ceres instead of whisper.
  • pytz and pyparsing are not bundled any more, drop the patch. (you still have the rm call)
  • The manage app doesn't exist any more, fix pkg_config() accordingly. Since upstream removed the manage script, we should probably use what their doc recommends.
  • Run build-index during pkg_config() instead of just creating an empty
    file. This allows us to drop the complex python file parsing.
  • Mention build-index in the postinst message.
  • Create the /var/{lib,log}/graphite-web directories which are
    referenced in local_settings.py.

@sbraz
Copy link
Member Author

sbraz commented Jun 24, 2018

@grobian I'm going to fix this PR to reuse your changes and add those from my ebuild that seem important.

Package-Manager: Portage-2.3.40, Repoman-2.3.9
* Sort dependencies.
* Do not require python[sqlite], it is never directly used.
* Do not require twisted or txAMQP, they are not directly
  used by graphite-web.
* Do not require zope-interface, it was removed in
  graphite-project/graphite-web@bed2d5b
* Do not require whisper. It is optional and in the future we could
  probably use carbon with ceres instead of whisper.
* pytz and pyparsing are not bundled any more, remove the call to rm.
* Use python_prepare_all instead of src_prepare.
* Install examples only once in the _all function. Use dodoc instead of
  doins. Do not compress them.
* Use the recommended way to configure the app instead of re-creating
  manage.py.
* Do not remove scripts from install.
* Run build-index during pkg_config() instead of just creating an empty
  file. This allows us to drop the complex python file parsing.
* Mention build-index in the postinst message. Only display the message
  for new installs.
* Create the /var/{lib,log}/graphite-web directories which are
  referenced in local_settings.py.
* Add the example database location to the FHS-style settings patch.
* Fix STATIC_ROOT in the FHS-style settings patch.
* Add missing || die to the ln call.
* bump to EAPI=7.

Package-Manager: Portage-2.3.40, Repoman-2.3.9
@sbraz
Copy link
Member Author

sbraz commented Jun 25, 2018

Re-pushed:

  • Sort dependencies.
  • Do not require python[sqlite], it is never directly used.
  • Do not require twisted or txAMQP, they are not directly
    used by graphite-web.
  • Do not require zope-interface, it was removed in
    graphite-project/graphite-web@bed2d5b
  • Do not require whisper. It is optional and in the future we could
    probably use carbon with ceres instead of whisper.
  • pytz and pyparsing are not bundled any more, remove the call to rm.
  • Use python_prepare_all instead of src_prepare.
  • Install examples only once in the _all function. Use dodoc instead of
    doins. Do not compress them.
  • Use the recommended way to configure the app instead of re-creating
    manage.py.
  • Do not remove scripts from install.
  • Run build-index during pkg_config() instead of just creating an empty
    file. This allows us to drop the complex python file parsing.
  • Mention build-index in the postinst message. Only display the message
    for new installs.
  • Create the /var/{lib,log}/graphite-web directories which are
    referenced in local_settings.py.
  • Add the example database location to the FHS-style settings patch.
  • Fix STATIC_ROOT in the FHS-style settings patch.
  • Add missing || die to the ln call.
  • bump to EAPI=7.
--- /usr/portage/net-analyzer/graphite-web/graphite-web-1.1.3.ebuild	2018-06-12 23:32:14.706939402 +0200
+++ graphite-web-1.1.3-r1.ebuild	2018-06-25 12:08:38.100518081 +0200
@@ -1,50 +1,43 @@
 # Copyright 1999-2018 Gentoo Foundation
 # Distributed under the terms of the GNU General Public License v2
 
-EAPI=6
+EAPI=7
 PYTHON_COMPAT=( python{2_7,3_{4,5,6}} )
 
 inherit distutils-r1 python-utils-r1 prefix
 
 DESCRIPTION="Enterprise scalable realtime graphing"
-HOMEPAGE="http://graphite.readthedocs.org/"
+HOMEPAGE="https://graphiteapp.org/"
 SRC_URI="mirror://pypi/${PN:0:1}/${PN}/${P}.tar.gz"
 
 LICENSE="Apache-2.0"
 SLOT="0"
 KEYWORDS="~amd64 ~x86"
-IUSE="+carbon mysql memcached postgres +sqlite"
-#ldap - needs bump of python-ldap to latest
-# ldap? ( dev-python/python-ldap[${PYTHON_USEDEP}] )
+IUSE="+carbon ldap mysql memcached postgres +sqlite"
 
 DEPEND=""
 RDEPEND="
 	carbon? ( dev-python/carbon[${PYTHON_USEDEP}] )
+	ldap? ( dev-python/python-ldap[${PYTHON_USEDEP}] )
+	memcached? ( dev-python/python-memcached[${PYTHON_USEDEP}] )
 	mysql? (
 		|| (
 			dev-python/mysql-python[${PYTHON_USEDEP}]
 			dev-python/mysqlclient[${PYTHON_USEDEP}]
 		)
 	)
-	memcached? ( dev-python/python-memcached[${PYTHON_USEDEP}] )
-	postgres? (
-		dev-python/psycopg:2[${PYTHON_USEDEP}]
-	)
-	dev-lang/python[sqlite?]
-	dev-python/cairocffi[${PYTHON_USEDEP}]
+	postgres? ( dev-python/psycopg:2[${PYTHON_USEDEP}] )
 	>=dev-python/django-1.8[sqlite?,${PYTHON_USEDEP}]
 	<dev-python/django-1.11.99[sqlite?,${PYTHON_USEDEP}]
 	>=dev-python/django-tagging-0.4.3[${PYTHON_USEDEP}]
+	dev-python/cairocffi[${PYTHON_USEDEP}]
 	dev-python/pyparsing[${PYTHON_USEDEP}]
 	dev-python/pytz[${PYTHON_USEDEP}]
 	dev-python/scandir[${PYTHON_USEDEP}]
 	dev-python/six[${PYTHON_USEDEP}]
-	dev-python/txAMQP[${PYTHON_USEDEP}]
-	dev-python/twisted[${PYTHON_USEDEP}]
 	dev-python/urllib3[${PYTHON_USEDEP}]
-	dev-python/zope-interface[${PYTHON_USEDEP}]
-	dev-python/whisper[${PYTHON_USEDEP}]
-	media-libs/fontconfig"
+	media-libs/fontconfig
+"
 
 PATCHES=(
 	# Do not install the configuration and data files. We install them
@@ -52,68 +45,52 @@
 	"${FILESDIR}"/${PN}-1.1.3-fhs-paths.patch
 )
 
-src_prepare() {
+python_prepare_all() {
 	# use FHS-style paths
 	export GRAPHITE_NO_PREFIX=yes
-	# make sure we don't use bundled stuff
-	rm -Rf webapp/graphite/thirdparty
-	distutils-r1_src_prepare
+	distutils-r1_python_prepare_all
 	eprefixify \
 		conf/graphite.wsgi.example \
 		webapp/graphite/local_settings.py.example
 }
 
+python_install_all() {
+	distutils-r1_python_install_all
+	keepdir /var/{lib,log}/${PN}
+	docinto examples
+	docompress -x "/usr/share/doc/${PF}/examples"
+	dodoc \
+		examples/example-graphite-vhost.conf \
+		conf/dashboard.conf.example \
+		conf/graphite.wsgi.example
+}
+
 python_install() {
 	distutils-r1_python_install \
 		--install-data="${EPREFIX}"/usr/share/${PN}
 
-	# make manage.py available from an easier location/name
-	# (missing from tarball)
-	dodir /usr/bin
-	cat > "${ED}"/usr/bin/${PN}-manage <<- EOS
-		#!/usr/bin/env python
-		import os
-		import sys
-
-		if __name__ == "__main__":
-		    os.environ.setdefault("DJANGO_SETTINGS_MODULE", "graphite.settings")
-
-		    from django.core.management import execute_from_command_line
-
-		    execute_from_command_line(sys.argv)
-	EOS
-	#mv "${D}"/$(python_get_sitedir)/graphite/manage.py \
-	#	"${ED}"/usr/bin/${PN}-manage || die
-	chmod 0755 "${ED}"/usr/bin/${PN}-manage || die
-	python_fix_shebang "${ED}"/usr/bin/${PN}-manage
-
 	insinto /etc/${PN}
 	newins webapp/graphite/local_settings.py.example local_settings.py
-	pushd "${D}"/$(python_get_sitedir)/graphite > /dev/null || die
-	ln -s ../../../../../etc/${PN}/local_settings.py local_settings.py
+	pushd "${D}/$(python_get_sitedir)"/graphite > /dev/null || die
+	ln -s ../../../../../etc/${PN}/local_settings.py local_settings.py || die
 	popd > /dev/null || die
-
-	insinto /usr/share/doc/${PF}/examples
-	doins \
-		examples/example-graphite-vhost.conf \
-		conf/dashboard.conf.example \
-		conf/graphite.wsgi.example
 }
 
 pkg_config() {
-	"${ROOT}"/usr/bin/${PN}-manage syncdb --noinput
-	local idx=$(grep 'INDEX_FILE =' "${EROOT}"/etc/graphite-web/local_settings.py 2>/dev/null)
-	if [[ -n ${idx} ]] ; then
-		idx=${idx##*=}
-		idx=$(echo ${idx})
-		eval "idx=${idx}"
-		touch "${ROOT}"/"${idx}"/index
-	fi
+	"${EROOT}"/usr/bin/django-admin.py migrate \
+		--settings=graphite.settings --run-syncdb
+	"${EROOT}"/usr/bin/build-index
 }
 
 pkg_postinst() {
-	einfo "You need to configure ${PN} to run with a WSGI server of your choice."
-	einfo "Don't forget to edit local_settings.py in ${EPREFIX}/etc/${PN}"
-	einfo "See http://graphite.readthedocs.org/en/latest/config-local-settings.html"
-	einfo "Run emerge --config =${PN}-${PVR} if this is a fresh install."
+	# Only display this for new installs
+	if [[ -z ${REPLACING_VERSIONS} ]]; then
+		einfo "You need to configure ${PN} to run with a WSGI server of your choice."
+		einfo "Don't forget to edit local_settings.py in ${EPREFIX}/etc/${PN}"
+		einfo "See https://graphite.readthedocs.org/en/latest/config-local-settings.html"
+		einfo "Run emerge --config =${PN}-${PVR} if this is a fresh install."
+		einfo ""
+		einfo "If you want to update the search index regularily, you should consider running"
+		einfo "the 'build-index' script in a crontab."
+	fi
 }

@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2018-06-25 10:35 UTC
Newest commit scanned: 9fad04b
Status: ✅ good

No issues found

Copy link
Member

@grobian grobian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once you renamed the build-index tool to ${PN}-build-index, feel free to push the change in a revbump (-r1).

pkg_config() {
"${EROOT}"/usr/bin/django-admin.py migrate \
--settings=graphite.settings --run-syncdb
"${EROOT}"/usr/bin/build-index
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this name is way too generic, please rename it to ${PN}-build-index and fix the mentionings it elsewhere

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I'll update the patch to install just this script and not the other 2:

/usr/bin/run-graphite-devel-server.py
/usr/bin/build-index.sh
/usr/bin/build-index

And I will rename it.

The revbump is here indeed :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the work, by the way, it's a tremendously complex package to unravel 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're welcome! It took a few hours indeed. Too bad we both worked on it.

I was waiting for python-ldap to support Python 3 (which required quite a bit of work as well) but I didn't know where I could reach you since you're not on IRC.
What is the best way to contact you? Email?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

email indeed works best, IRC is really hard to combine for me

@@ -36,7 +36,7 @@ Install FHS-style paths
#INDEX_FILE = '/opt/graphite/storage/index' # Search index file
+CONF_DIR = '@GENTOO_PORTAGE_EPREFIX@/etc/graphite-web'
+STORAGE_DIR = '@GENTOO_PORTAGE_EPREFIX@/var/lib/carbon'
+STATIC_ROOT = '@GENTOO_PORTAGE_EPREFIX@/usr/share/graphite-web'
+STATIC_ROOT = '@GENTOO_PORTAGE_EPREFIX@/usr/share/graphite-web/webapp/content'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice catch, seems I got this fixed in my setup with puppet, but didn't notice the default was wrong

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On my setup, nginx is serving the static files so I don't think it matters. I assume it is not used unless graphite itself is serving those:

Alternatively, static files can be served directly by the Graphite webapp if you install the whitenoise Python package.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember having issues with it, because my graphite-web instance is running on a different machine than the machine running nginx, so I had to forward proxy it because I couldn't get graphite-web to do it.

@sbraz
Copy link
Member Author

sbraz commented Jun 25, 2018

@grobian could we use elog instead of einfo? It would be nice to log those messages and it seems that I don't even see einfo messages when I run emerge with --jobs > 1.

@grobian
Copy link
Member

grobian commented Jun 25, 2018

that's fine

@sbraz sbraz deleted the graphite branch June 26, 2018 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assigned PR successfully assigned to the package maintainer(s). do not merge Please DO NOT MERGE this PR. It will not be assigned but it will be scanned by CI. no bug found No Bug/Closes found in the commits.
Projects
None yet
4 participants