www-apps/redmine: New version 3.3.3 #4550

winterheart · 2017-05-06T04:18:32Z

Fixed slot dependencies, startup script, added myself to proxied maintainers (Bug #590646)

Package-Manager: Portage-2.3.3, Repoman-2.3.1

gentoo-repo-qa-bot · 2017-05-06T04:20:07Z

Pull Request assignment

Areas affected: ebuilds
Packages affected: www-apps/redmine

www-apps/redmine: @gentoo/proxy-maint (maintainer needed)

mgorny

@gentoo/ruby, could you also take a look?

mgorny · 2017-05-07T08:03:47Z

www-apps/redmine/files/redmine-3.initd

+
+start() {
+	ebegin "Starting redmine"
+	cd "${REDMINE_DIR}"


Missing error handling. You can just add && here.

mgorny · 2017-05-07T08:04:06Z

www-apps/redmine/files/redmine-3.initd

+
+start_pre() {
+	if [ ! -e "${REDMINE_DIR}/config/initializers/secret_token.rb" ] ; then
+		eerror "Execute the following command to initlize environment:"


mgorny · 2017-05-07T08:08:15Z

www-apps/redmine/redmine-3.3.3.ebuild

+
+	rm Gemfile || die
+
+	echo "CONFIG_PROTECT=\"${EPREFIX}${REDMINE_DIR}/config\"" > "${T}/50${PN}"


Missing ||die. Also, I think the heredoc syntax would be more readable here (less escapes, shorter lines).

mgorny · 2017-05-07T08:10:11Z

www-apps/redmine/redmine-3.3.3.ebuild

+	use ldap || rm app/models/auth_source_ldap.rb || die
+
+	# Make it work
+	sed -i -e "1irequire 'request_store'" app/controllers/application_controller.rb || die


Are all those changes still required? They really ought to be carried via a patch. This sed will never fail, even if upstream changed the relevant code — either fixing the issue, or breaking the fix.

mgorny · 2017-05-07T08:10:49Z

www-apps/redmine/redmine-3.3.3.ebuild

+all_ruby_install() {
+	dodoc doc/{CHANGELOG,INSTALL,README_FOR_APP,RUNNING_TESTS,UPGRADING}
+	rm -r doc || die
+	dodoc README.rdoc


Any reason not to combine this into a single dodoc and rm? ;-)

mgorny · 2017-05-07T08:11:53Z

www-apps/redmine/redmine-3.3.3.ebuild

+	rm README.rdoc || die
+
+	keepdir /var/log/${PN}
+	dosym /var/log/${PN}/ "${REDMINE_DIR}/log"


Please use a relative symlink here. Since REDMINE_DIR is unconditionally defined in the ebuild, you can do that safely.

mgorny · 2017-05-07T08:13:57Z

www-apps/redmine/redmine-3.3.3.ebuild

+
+pkg_postinst() {
+	einfo
+	if [ -e "${EPREFIX}${REDMINE_DIR}/config/initializers/session_store.rb" -o -e "${EPREFIX}${REDMINE_DIR}/config/initializers/secret_token.rb" ]; then


Don't ye-ol-sh syntax in bash. Also this ought to use EROOT, i.e.:

if [[ -e ${EROOT%/}... || -e ${EROOT%/}...

winterheart · 2017-05-24T17:11:13Z

@mgorny done.

mgorny

A few minor nits.

mgorny · 2017-05-24T17:19:28Z

www-apps/redmine/files/additional_environment.rb

+# Place log-files to /var/log/redmine
+config.logger = Logger.new(Rails.root.join("/var/log/redmine",Rails.env + ".log"), 0, 10485760)
+config.log_level= :info
+


Stray empty line!

mgorny · 2017-05-24T17:20:47Z

www-apps/redmine/files/redmine-3.initd

+		eerror
+		return 1
+	fi
+	if [ ! -d /var/run/redmine ]; then


@gentoo/openrc, can't checkpath be run unconditionally in this context?

mgorny · 2017-05-24T17:21:10Z

www-apps/redmine/files/redmine-3.initd

+
+start() {
+	ebegin "Starting redmine"
+	start-stop-daemon --start --quiet --user ${REDMINE_USER}:${REDMINE_GROUP} \


I think it'd be reasonable to quote user&group too, at least for consistency.

mgorny · 2017-05-24T17:21:42Z

www-apps/redmine/metadata.xml

@@ -3,7 +3,16 @@
 <pkgmetadata>
 <longdescription lang="en">


Could you drop empty longdesc, please?

mgorny · 2017-05-24T17:22:32Z

www-apps/redmine/redmine-3.3.3.ebuild

+	# bug #406605
+	rm .{git,hg}ignore || die
+
+	cat > ${T}/50${PN} <<-EOF


mgorny · 2017-05-24T17:23:02Z

www-apps/redmine/redmine-3.3.3.ebuild

+	# remove ldap staff module if disabled to avoid #413779
+	use ldap || rm app/models/auth_source_ldap.rb || die
+
+	epatch "${FILESDIR}/${P}_requires.patch"


Please use eapply in EAPI 6.

mgorny · 2017-05-24T17:23:49Z

www-apps/redmine/redmine-3.3.3.ebuild

+
+pkg_postinst() {
+	einfo
+	if [ -e "${EROOT%/}${REDMINE_DIR}/config/initializers/session_store.rb" ] || [ -e "${EROOT%/}${REDMINE_DIR}/config/initializers/secret_token.rb" ]; then


Use [[ ... ]] in bash, and then you can put || inside. Also, line wrapping would probably be beneficial to the reader ;-).

mgorny · 2017-05-24T17:24:48Z

www-apps/redmine/redmine-3.3.3.ebuild

+}
+
+pkg_config() {
+	if [ ! -e "${EROOT%/}${REDMINE_DIR}/config/database.yml" ]; then


[[ ... ]] everywhere.

mgorny · 2017-05-24T17:25:46Z

www-apps/redmine/redmine-3.3.3.ebuild

+
+pkg_config() {
+	if [ ! -e "${EROOT%/}${REDMINE_DIR}/config/database.yml" ]; then
+		eerror "Copy ${EROOT%/}${REDMINE_DIR}/config/database.yml.example to ${EROOT%/}${REDMINE_DIR}/config/database.yml"


I think this line can end up veery long. I'd suggest wrapping at 80, or approximate 80 ;-).

winterheart · 2017-05-26T10:39:36Z

@mgorny done.

mgorny · 2017-06-03T10:28:21Z

www-apps/redmine/files/redmine-3.initd

+}
+
+start_pre() {
+	if [[ ! -e "${REDMINE_DIR}/config/initializers/secret_token.rb" ]] ; then


init.d scripts are POSIX-ish sh, so you can't use [[ ... ]].

mgorny · 2017-06-03T10:30:10Z

www-apps/redmine/redmine-3.3.3.ebuild

+pkg_postinst() {
+	einfo
+	if [[ -e "${EROOT%/}${REDMINE_DIR}/config/initializers/session_store.rb" \
+	|| -e "${EROOT%/}${REDMINE_DIR}/config/initializers/secret_token.rb" ]]; \


Pro-tip: ; is equivalent to newline, so really no point in doing ; \ when you can leave it out.

winterheart · 2017-06-13T09:23:33Z

@mgorny Done

mgorny · 2017-06-14T21:36:41Z

www-apps/redmine/redmine-3.3.3.ebuild

+		RAILS_ENV="${RAILS_ENV}" ${RUBY} -S rake db:migrate || die
+		einfo "Populating database with default configuration data."
+		RAILS_ENV="${RAILS_ENV}" ${RUBY} -S rake redmine:load_default_data || die
+		chown redmine:redmine "${EROOT%/}var/log/redmine/*.log"


Any reason you're not ||dieing here?

mgorny · 2017-06-14T21:37:32Z

www-apps/redmine/redmine-3.3.3.ebuild

+pkg_postinst() {
+	einfo
+	if [[ -e "${EROOT%/}${REDMINE_DIR}/config/initializers/session_store.rb" \
+	|| -e "${EROOT%/}${REDMINE_DIR}/config/initializers/secret_token.rb" ]];


Semicolon at end of the line is redundant ;-).

mgorny · 2017-06-14T21:38:25Z

www-apps/redmine/redmine-3.3.3.ebuild

+	passenger? ( www-apache/passenger )
+	"
+
+#ruby_add_bdepend ">=dev-ruby/rdoc-2.4.2


Maybe add a comment explaining why this is commented out.

mgorny · 2017-06-14T21:39:22Z

www-apps/redmine/redmine-3.3.3.ebuild

+#		>=dev-ruby/shoulda-3.3.2
+#		>=dev-ruby/mocha-0.13.3
+#		>=dev-ruby/capybara-2.0.0
+#		<dev-ruby/nokogiri-1.6.0


I suppose this can't happen considering the above >= dep… so is this outdated or…?

mgorny · 2017-06-14T21:39:49Z

www-apps/redmine/redmine-3.3.3.ebuild

+#IUSE="ldap openid imagemagick postgres sqlite mysql fastcgi passenger"
+IUSE="imagemagick fastcgi ldap markdown passenger"
+
+# nokogiri ruby 2.1 >= 1.7.0, else 1.6.8


Considering we don't support ruby < 2.1, time to bump the dep?

mgorny · 2017-06-14T21:40:17Z

www-apps/redmine/files/redmine-3.initd

+		--user "${REDMINE_USER}:${REDMINE_GROUP}" \
+		--pidfile "${REDMINE_PIDFILE}" \
+		--exec /usr/bin/ruby "${REDMINE_DIR}"/bin/rails server -- \
+		--daemon --environment=${RAILS_ENV} \


Don't you want to quote this and the two variables below?

winterheart · 2017-06-24T11:11:16Z

@mgorny done

mgorny

You still didn't address two of my comments (GitHub shows them in 'files changed' section).

Also, please update it for 3.3.4 (or 3.4.0).

mgorny · 2017-07-05T09:01:03Z

(and rebase)

Fixed slot dependencies, startup script (Bug #603452), added new log location and logrotate, added myself to proxied maintainers (Bug #590646). Package-Manager: Portage-2.3.3, Repoman-2.3.1

winterheart · 2017-07-05T10:52:16Z

@mgorny done.
I decided to bump to all supported versions - 3.2.7, 3.3.4, 3.4.0, according to upstream release policy.

gentoo-repo-qa-bot · 2017-07-05T11:02:42Z

😞 The QA check for this pull request has found the following issues:

mgorny · 2017-07-14T21:53:13Z

I'm going to trust you it's all okay now. I can't stand looking at it anymore ;-).

gentoo-repo-qa-bot added maintainer-needed There is at least one affected package with no maintainer. Review it if you can. assigned PR successfully assigned to the package maintainer(s). labels May 6, 2017

mgorny requested changes May 7, 2017

View reviewed changes

winterheart force-pushed the redmine branch from 46aa6c3 to 4b3524d Compare May 24, 2017 17:10

mgorny requested changes May 24, 2017

View reviewed changes

winterheart force-pushed the redmine branch from 4b3524d to 4aad860 Compare May 26, 2017 10:39

mgorny requested changes Jun 3, 2017

View reviewed changes

winterheart force-pushed the redmine branch from 4aad860 to 62596e8 Compare June 13, 2017 09:15

mgorny reviewed Jun 14, 2017

View reviewed changes

winterheart force-pushed the redmine branch from 62596e8 to b432b75 Compare June 24, 2017 11:06

mgorny requested changes Jul 5, 2017

View reviewed changes

www-apps/redmine: Bump to 3.2.7, 3.3.4, 3.4.0.

53229f5

Fixed slot dependencies, startup script (Bug #603452), added new log location and logrotate, added myself to proxied maintainers (Bug #590646). Package-Manager: Portage-2.3.3, Repoman-2.3.1

winterheart force-pushed the redmine branch from f92c553 to 53229f5 Compare July 5, 2017 10:48

mgorny approved these changes Jul 14, 2017

View reviewed changes

gentoo-bot closed this in 118747f Jul 14, 2017


		rm Gemfile \|\| die

		echo "CONFIG_PROTECT=\"${EPREFIX}${REDMINE_DIR}/config\"" > "${T}/50${PN}"

www-apps/redmine: New version 3.3.3 #4550

www-apps/redmine: New version 3.3.3 #4550

Conversation

winterheart commented May 6, 2017

gentoo-repo-qa-bot commented May 6, 2017

mgorny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winterheart commented May 24, 2017

mgorny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winterheart commented May 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winterheart commented Jun 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winterheart commented Jun 24, 2017

mgorny left a comment

Choose a reason for hiding this comment

mgorny commented Jul 5, 2017

winterheart commented Jul 5, 2017

gentoo-repo-qa-bot commented Jul 5, 2017

mgorny commented Jul 14, 2017