www-servers/nginx-unit: Added 'ruby' use flag #9278

rseichter · 2018-07-18T19:20:07Z

nginx-unit-1.3-r1 adds the 'ruby' use flag, which enables Ruby support for NGINX Unit. ~~No further changes are introduced with this ebuild update.~~ EDIT: This ebuild also includes fixes for the modules directory location and REQUIRED_USE.

mgorny · 2018-07-27T18:13:49Z

www-servers/nginx-unit/nginx-unit-1.3-r1.ebuild

+DESCRIPTION="A dynamic web and application server"
+HOMEPAGE="https://unit.nginx.org"
+SRC_URI="https://unit.nginx.org/download/unit-${PV}.tar.gz -> ${P}.tar.gz"
+LICENSE="Apache-2.0"


Please group variables like in the template:

DESCRIPTION="" HOMEPAGE="" SRC_URI="" LICENSE="" SLOT="" KEYWORDS="" IUSE="" [REQUIRED_USE] DEPEND/RDEPEND in any order other stuff

mgorny · 2018-07-27T18:14:33Z

www-servers/nginx-unit/nginx-unit-1.3-r1.ebuild

+
+DESCRIPTION="A dynamic web and application server"
+HOMEPAGE="https://unit.nginx.org"
+SRC_URI="https://unit.nginx.org/download/unit-${PV}.tar.gz -> ${P}.tar.gz"


I don't think you actually need to rename it. Plus, you can put unit-${PV} into MY_P to avoid having to repeat it twice.

I made the MY_P change, but if it is OK with you I'd rather stick with renaming the tarball during download. "unit-x.y.tar.gz" is not very descriptive.

Sure, it's not a big deal.

Thanks. As far as I can tell, I complied with your other change requests. Interestingly, I only changed the Ruby support (as you can see from the diffs shown in my original comment), and the last time the ebuild was accepted by the reviewer. I guess it just goes to show that different Gentoo devs have different preferences. 😉

mgorny · 2018-07-27T18:15:15Z

www-servers/nginx-unit/nginx-unit-1.3-r1.ebuild

+src_install() {
+	default
+	keepdir /var/lib/${PN}
+	fperms 0770 /var/lib/${PN}


Use diropts to control mode when creating it, instead of changing post-creation.

mgorny

We all learn, and we teach contributors new stuff when we learn it. Plus, we sometimes miss stuff.

  unit modules directory:       "/usr/modules"

This is not correct FHS directory.

Also, please make separate commits for irrelevant changes.

mgorny · 2018-07-28T06:27:13Z

www-servers/nginx-unit/nginx-unit-1.3-r1.ebuild

+REQUIRED_USE="|| ( ${IUSE} )"
+
+DEPEND="perl? ( dev-lang/perl:= )
+	python? ( dev-lang/python:= )


Depending on Python directly is forbidden. Use python-single-r1 to provide proper choice for Python version.

These dependencies are only a way to get the user to choose at least one of the supported application languages. Please see here for an explanation I added to the initial ebuild. It does not matter which version of Python the user has selected, the build will use the one it finds.

I hope to be able to support multiple distinct Python versions in future versions, but as of now I only want to get Ruby support out.

python-single-r1 is for single implementation.

mgorny · 2018-07-28T06:27:50Z

www-servers/nginx-unit/nginx-unit-1.3-r1.ebuild

+
+src_install() {
+	default
+	diropts -m 0770


By the way, I really don't understand the purpose of this. Why would you make it writable to root group?

So that members of the root group can place shared command scripts and JSON files into /var/lib/nginx-unit.

Why would root group have members at all? But whatev. It looks weird design but doesn't harm.

rseichter · 2018-07-28T09:49:21Z

Re module installation location: Would /usr/share/nginx-unit/modules be the correct directory to use?

rseichter · 2018-07-28T10:02:21Z

BTW, why is this pull request not marked with the "self-maintained" label? Is it because my GitHub email address does not match the maintainer email address contained in the ebuild?

mgorny · 2018-07-28T11:28:15Z

Re module installation location: Would /usr/share/nginx-unit/modules be the correct directory to use?

Nope, /usr/share is for arch-independent stuff. /usr/lib or /usr/$(get_libdir) are both correct here.\

BTW, why is this pull request not marked with the "self-maintained" label? Is it because my GitHub email address does not match the maintainer email address contained in the ebuild?

Rather because the database used for that is updated semi-manually. I'll retry.

gentoo-bot · 2018-07-28T11:40:10Z

Pull Request assignment

Areas affected: ebuilds
Packages affected: www-servers/nginx-unit

www-servers/nginx-unit: @rseichter, @gentoo/proxy-maint

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.

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

rseichter · 2018-07-28T13:08:05Z

/usr/lib or /usr/$(get_libdir) are both correct here.

Fixed. I also use python-single-r1 now (hopefully in the correct fashion, I had a peek at some existing ebuilds).

mgorny · 2018-07-28T13:11:40Z

www-servers/nginx-unit/nginx-unit-1.3-r1.ebuild

@@ -4,6 +4,10 @@
 EAPI=7

 MY_P="unit-${PV}"
+MY_IUSE="perl python ruby"


You don't need those vars before inherit. Please try to keep them near their first use, otherwise it's hard to find them; i.e. MY_P above DESCRIPTION, MY_IUSE above IUSE.

mgorny · 2018-07-28T13:11:50Z

www-servers/nginx-unit/nginx-unit-1.3-r1.ebuild

+MY_IUSE="perl python ruby"
+PYTHON_COMPAT=(python2_7 python3_{3,4,5,6})
+
+inherit multilib python-single-r1


multilib is not needed since EAPI 6.

mgorny · 2018-07-28T13:12:21Z

www-servers/nginx-unit/nginx-unit-1.3-r1.ebuild

@@ -12,23 +16,24 @@ SRC_URI="https://unit.nginx.org/download/${MY_P}.tar.gz -> ${P}.tar.gz"
 LICENSE="Apache-2.0"
 SLOT="0"
 KEYWORDS="~amd64"
-IUSE="perl python ruby"
+IUSE="${MY_IUSE}"
 REQUIRED_USE="|| ( ${IUSE} )"


I think you want to use MY_IUSE here.

mgorny · 2018-07-28T13:12:37Z

www-servers/nginx-unit/nginx-unit-1.3-r1.ebuild

 REQUIRED_USE="|| ( ${IUSE} )"

 DEPEND="perl? ( dev-lang/perl:= )
-	python? ( dev-lang/python:= )
+	python? ( ${PYTHON_DEPS} )


You also need REQUIRED_USE with PYTHON_REQUIRED_USE inside.

rseichter · 2018-07-28T13:42:13Z

Next round of whack-a-mole done. 😅

rseichter · 2018-07-28T15:22:52Z

@mgorny : Looks like I cannot use REQUIRED_USE="|| ( perl ${PYTHON_REQUIRED_USE} ruby )". Any suggestions how this can be resolved?

mgorny · 2018-07-28T18:48:08Z

|| ( perl python ruby ) python? ( ${PYTHON_REQUIRED_USE} )

Remember to make python-single-r1_pkg_setup also USE-conditional.

rseichter · 2018-07-28T19:06:49Z

And here I was, thinking that adding support for a single Python version was simple...

mgorny · 2018-07-28T20:49:43Z

www-servers/nginx-unit/nginx-unit-1.3-r1.ebuild

+KEYWORDS="~amd64"
+MY_IUSE="perl python ruby"
+IUSE="${MY_IUSE}"
+REQUIRED_USE="|| ( perl python ruby ) python? ( ${PYTHON_REQUIRED_USE} )"


Don't you want to reuse MY_IUSE here?

I thought about it myself, then decided to use what you wrote verbatim. I can of course change the REQUIRED_USE definition if you like.

mgorny · 2018-07-29T06:18:53Z

It's your choice whether you want helper variable or not but you should be consistent throughout the ebuild.

Now the last part of whack-a-mole: please git rebase -i to turn the commits into logical changes on top of master (i.e. only changes that are meaningful, fixes to your earlier commits should be squashed on top of those commits). Also please use GLEP 66 prefixes www-servers/nginx-unit: on every commit and verify the final state with repoman full -d.

Package-Manager: Portage-2.3.40, Repoman-2.3.9

rseichter · 2018-07-29T10:39:48Z

Done squashing. I also just updated the initial description of this PR.

mgorny · 2018-07-29T19:56:05Z

I think we didn't understand each other. Your first commit looks good, since other changes (besides adding USE=ruby) are minor. However, your second commit mixes modules change with half-broken Python changes which shouldn't enter ::gentoo. It would be perfect if you could edit the commits so that moduledir changes and Python changes (in one commit) would be separate but if that's too hard, just merge the two into one commit (but mention both in summary line since both are important, and you need to fit in 69 chars).

rseichter · 2018-07-29T20:19:57Z

I hope the current state is acceptable. The only other option is to squash it all into one commit, since after all it is only one file that was changed.

mgorny · 2018-07-29T20:32:37Z

Thanks. I'll reword that last commit message bit not to bother you more with trivial stuff, test it and merge it if builds fine.

mgorny · 2018-07-29T20:35:08Z

Hrm…

>>> /usr/lib64/ruby.unit.so
>>> /usr/lib64/perl.unit.so
>>> /usr/lib64/python.unit.so

Could we please move it to a subdirectory? It really looks kinda weird mixed with regular libs.

rseichter · 2018-07-29T20:35:16Z

Thank you for your patience. It is obvious that I am new to this, and I sometimes had to wing it because I did not immediately understand what you expected.

Also added pkg_setup() to conditionally support Python and fixed contents of REQUIRED_USE.

gentoo-repo-qa-bot · 2018-07-29T21:34:47Z

Pull request CI report

Report generated at: 2018-07-29 21:34 UTC
Newest commit scanned: 02b5800
Status: ✅ good

No issues found

graaff · 2018-07-29T09:02:21Z

www-servers/nginx-unit/nginx-unit-1.3-r1.ebuild

+
+DEPEND="perl? ( dev-lang/perl:= )
+	python? ( dev-lang/python:= )
+	ruby? ( dev-lang/ruby:= )"


Ruby has similar limitations on depending on dev-lang/ruby directly, and for the same reason as Python: it never does what you expect it to do.

First of all you assume that all ruby versions will always be compatible with the C code in the module, but ruby does not make this guarantee. E.g. code can compile against 2.4 but not 2.5. To avoid issues here you test against specific ruby versions and list these in USE_RUBY and use the ruby eclasses to handle dependencies and building against these versions.

As far as I can tell nginx-unit compiles a shared object file for ruby. This can easily lead to breakage when ruby versions are switched or removed. the := in the ruby dependency does not guard against this because the package manager doesn't know which specific version of ruby this package was compiled against. This will also apply to python.

For the ruby side we support this via ruby-ng.eclass, but unfortunately not in a way that works well with other languages that have similar needs like python.

rseichter · 2018-07-30T09:26:14Z

@mgorny and @graaff: I appreciate your comments and I think I can understand your motivation, but this is getting out of hand. I seem to be required to find ways of overcoming the idiosyncrasies of NGINX Unit's design (see also the discussion about PHP in this pull request) to force it into a Gentoo mould, and the amount of time it takes me, given my limited experience with ebuilds (this is my first one), is disproportionately high.

Would it be better if someone like you guys laid the proper foundation for this ebuild? I simply don't know when to use which eclass, what limitations apply, etc., and I don't want to waste my time or yours by continuing this back-and-forth.

mgorny · 2018-07-30T19:55:32Z

I'm afraid you simply choose a very hard ebuild to begin with. I've only asked you to fix stuff I knew how to help you solve. Hopefully @graaff will elaborate more on how Ruby support can be solved. I should probably note that PHP requires some magic as well but since it's not a regression and I don't know how to do it, I won't bother you with it.

rseichter · 2018-07-30T20:14:45Z

@mgorny: Am I correct to assume that the Python issues are now all fixed? If so, I can take out the Ruby support (even though it was the original motivation) and this PR can finally be merged.

If @graaf wants to later provide detailed instructions on how Ruby support can be added, I can look into it again, but right now I want this over and done with. I am neither masochistic nor did I sign anything, and I'd be content with a 90% solution.

mgorny · 2018-07-30T20:22:34Z

We have discussed this and I'm going to merge this as-is. We can revisit Ruby/PHP issues later, either when someone has a good idea how to solve it or you figure it out yourself. Thanks for your patience.

gentoo-bot added assigned PR successfully assigned to the package maintainer(s). no bug found No Bug/Closes found in the commits. labels Jul 18, 2018

mgorny requested changes Jul 27, 2018

View reviewed changes

mgorny requested changes Jul 28, 2018

View reviewed changes

mgorny removed the assigned PR successfully assigned to the package maintainer(s). label Jul 28, 2018

gentoo-bot added assigned PR successfully assigned to the package maintainer(s). no bug found No Bug/Closes found in the commits. and removed no bug found No Bug/Closes found in the commits. labels Jul 28, 2018

mgorny removed the assigned PR successfully assigned to the package maintainer(s). label Jul 28, 2018

gentoo-bot added self-maintained The PR changes only packages that are maintained by the submitter (i.e. no need to ask anybody else) assigned PR successfully assigned to the package maintainer(s). and removed no bug found No Bug/Closes found in the commits. labels Jul 28, 2018

mgorny reviewed Jul 28, 2018

View reviewed changes

www-servers/nginx-unit: Added 'ruby' use flag

efd48c7

Package-Manager: Portage-2.3.40, Repoman-2.3.9

www-servers/nginx-unit: Fixed modules directory, Python dependencies

02b5800

Also added pkg_setup() to conditionally support Python and fixed contents of REQUIRED_USE.

graaff reviewed Jul 30, 2018

View reviewed changes

mgorny approved these changes Jul 30, 2018

View reviewed changes

gentoo-bot closed this in a781960 Jul 30, 2018

rseichter deleted the www-servers/nginx-unit branch July 30, 2018 20:34

www-servers/nginx-unit: Added 'ruby' use flag #9278

www-servers/nginx-unit: Added 'ruby' use flag #9278

Conversation

rseichter commented Jul 18, 2018 • edited

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

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

rseichter commented Jul 28, 2018

rseichter commented Jul 28, 2018

mgorny commented Jul 28, 2018

gentoo-bot commented Jul 28, 2018

rseichter commented Jul 28, 2018

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

rseichter commented Jul 28, 2018

rseichter commented Jul 28, 2018

mgorny commented Jul 28, 2018

rseichter commented Jul 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgorny commented Jul 29, 2018

rseichter commented Jul 29, 2018

mgorny commented Jul 29, 2018

rseichter commented Jul 29, 2018

mgorny commented Jul 29, 2018

mgorny commented Jul 29, 2018

rseichter commented Jul 29, 2018

gentoo-repo-qa-bot commented Jul 29, 2018

Choose a reason for hiding this comment

rseichter commented Jul 30, 2018

mgorny commented Jul 30, 2018

rseichter commented Jul 30, 2018

mgorny commented Jul 30, 2018

rseichter commented Jul 18, 2018 •

edited