Page MenuHomeFreeBSD

www/unit: Separate and complete language modules
ClosedPublic

Authored by lwhsu on Nov 24 2017, 3:32 PM.

Details

Summary
  • Separate language modules into individual ports
  • Complete Go module

Diff Detail

Repository
rP FreeBSD ports repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

lwhsu created this revision.Nov 24 2017, 3:32 PM
mat added a comment.Nov 24 2017, 6:33 PM

Don't create the py3- port, you will not be allowed to commit it. (And it will be useless soon.)

osa added a subscriber: osa.Nov 25 2017, 1:09 AM
osa added a comment.Nov 25 2017, 1:18 AM

Hi Li-Wen,

thanks very much for this job, it looks great!

Since you've created the ports with suffixes with versions -php7[0-2], can we use the same naming convention for unit module for php56, i.e. www/unit-php56?
Also, I'd prefer to see a modules for every version of python we have in our ports tree. Hope we can use the same naming convention for pytho modules, i.e. www/unit-python[27,34-36].

lwhsu updated this revision to Diff 35757.Nov 25 2017, 7:40 AM
  • Remove py3-unit-python
  • Rename unit-php to unit-php56
lwhsu added a comment.Nov 25 2017, 7:43 AM

unit-php -> unit-php56 renaming is done.
@osa , I think @mat means D12464 , it looks we don't need to create individual port for different python version after it gets committed.

lwhsu added a reviewer: osa.Nov 25 2017, 7:44 AM
osa added a comment.Nov 25 2017, 2:30 PM

Hi Li-Wen,

we definitely don't need to create a separate port for every version of python or even php if we can do that with one version of a port.
In this case - how can we install a couple of unit modules to support two different versions of python, i.e. python27 and python36, can we do so with current version of www/py-unit-python?

lwhsu added a comment.Nov 25 2017, 4:08 PM
In D13227#275997, @osa wrote:

we definitely don't need to create a separate port for every version of python or even php if we can do that with one version of a port.
In this case - how can we install a couple of unit modules to support two different versions of python, i.e. python27 and python36, can we do so with current version of www/py-unit-python?

If I understand correctly, the new infra will automatically generate packages with 2 different versions of python (default versions of python2 and python3), so we will have py27-unit-python and py36-unit-python packages from one port directory.
Please note that in py-unit-python, I use PYTHON_SUFFIX in the module name so there will be no conflict for two modules co-exist.

However, I also like to have pre-built packages for each supported version of python in the ports tree. I will join the discussion in that differential to express my opinions.

If that differential takes too long to land, I think we can have py3-unit-python (or even py34-, py35-, py36-) added first.

How do you think? Let's have this patch committed first, for getting better Go and PHP support.

osa added a comment.Nov 26 2017, 1:13 AM
In D13227#275997, @osa wrote:

we definitely don't need to create a separate port for every version of python or even php if we can do that with one version of a port.
In this case - how can we install a couple of unit modules to support two different versions of python, i.e. python27 and python36, can we do so with current version of www/py-unit-python?

If I understand correctly, the new infra will automatically generate packages with 2 different versions of python (default versions of python2 and python3), so we will have py27-unit-python and py36-unit-python packages from one port directory.
Please note that in py-unit-python, I use PYTHON_SUFFIX in the module name so there will be no conflict for two modules co-exist.

What about two other versions of python? In current version of www/unit we can create the unit modules for all four versions of python. How can we do so with your solution?

However, I also like to have pre-built packages for each supported version of python in the ports tree. I will join the discussion in that differential to express my opinions.
If that differential takes too long to land, I think we can have py3-unit-python (or even py34-, py35-, py36-) added first.

This is what I'd like to see in the ports tree for every language and it's version (at this moment we have 4 versions of php, 4 versions of python as well and go):

  • an ability to build a module from ports tree;
  • an ability to use/install a pre-built a package as well.

How do you think? Let's have this patch committed first, for getting better Go and PHP support.

So, I don't think I can agree with this approach now. Let's create a complete solution first, then commit it into the ports tree.
Please let me know if you have any questions.

lwhsu updated this revision to Diff 35798.Nov 26 2017, 1:58 PM
  • Add back py3*-unit-python, until we have better support in the ports infra
lwhsu added a comment.Nov 26 2017, 2:05 PM

@osa , I agree with you, I think having py3*-unit-python for now is a good way for improving unit and its modules without regressions (comparing with what we have in ports currently)

@mat, I am happy to work with you for multiple python versions support, and I can take care of modifying py-unit-python and removing py3*-unit-python after D12464 committed.

lwhsu updated this revision to Diff 36011.Nov 30 2017, 5:30 PM
  • Use python flavors & allflavors to build modules for all python versions
mat added inline comments.Nov 30 2017, 5:46 PM
www/py-unit-python/Makefile
5 ↗(On Diff #36011)

Side question, does it really need the PKGNAMESUFFIX ?

I mean, it already going to be named pyXY-unit, unless there is another port that generates those packages, I think the -python bit should be dropped.

7 ↗(On Diff #36011)

Remove ${PYTHON_VER} it is obvious by the pyXY- prefix.

9 ↗(On Diff #36011)

s/?=/=/

10 ↗(On Diff #36011)

Unless there are other ports actually using the non default Python versions, I don't think allflavors is needed here.

12 ↗(On Diff #36011)
PLIST_FILES= libexec/unit/modules/python${PYTHON_SUFFIX}.unit.so
lwhsu added inline comments.Dec 1 2017, 2:34 PM
www/py-unit-python/Makefile
5 ↗(On Diff #36011)

I'd like to keep consistency between unit modules, unit-go, unit-php and unit-python.
How about moving to suffix like: unit-python-py24 or even unit-py24?

7 ↗(On Diff #36011)

OK, though I thought it's no harm to mention again :)

10 ↗(On Diff #36011)

I originally did this, but @osa (www/unit maintainer) would like to keep the old behavior of building all python modules for unit.

12 ↗(On Diff #36011)

I checked other PLIST_FILES usage in the ports tree, we use %% for PLIST_SUB. Did I miss anything?

www/unit-php56/Makefile
6 ↗(On Diff #36011)

I guess I should also remove ${PHP_VERSION:R}

lwhsu updated this revision to Diff 36048.Dec 1 2017, 2:37 PM
  • Remove unnecessary ${PYTHON_VER} and ${PHP_VERSION:R}
  • Remove unnecessary ?= www/py-unit-python was a slave of www/unit but also master of www/py3-unit-python
lwhsu marked 4 inline comments as done.Dec 1 2017, 2:38 PM
mat added inline comments.Dec 1 2017, 2:41 PM
www/py-unit-python/Makefile
5 ↗(On Diff #36011)

well, pyXY-unit already says python, no need to repeat it.

10 ↗(On Diff #36011)

There is no such behavior. With the current code it only builds the first selected version.

12 ↗(On Diff #36011)

Just because a few other did a bad thing does not mean you have to repeat it.
The %%FOO%% placeholders are for the pkg-plist file, because it is a static file and variables need to be carried there.
Here, in the Makefile, you can access the variable directly, use it.

lwhsu added inline comments.Dec 1 2017, 3:04 PM
www/py-unit-python/Makefile
5 ↗(On Diff #36011)

I thought is pyXX- is the flavor and unit-python is the port name. I prefer this form, honestly.
If you insist on removing -python suffix, I will do so.

The name I wanted most is unit-pythonXX, though it is not consistent with other ports in the tree.

10 ↗(On Diff #36011)

Oh, right, what I remembered is wrong. Just keeping the ability to build modules for other python versions is fine.

lwhsu updated this revision to Diff 36049.Dec 1 2017, 3:05 PM
  • Remove unnecessary allflavors
lwhsu updated this revision to Diff 36050.Dec 1 2017, 3:15 PM
  • Directly use Makefile variables in PLIST_FILES
mat added inline comments.Dec 1 2017, 3:30 PM
www/py-unit-python/Makefile
5 ↗(On Diff #36011)

Here, in the end, with what is currently in the review, we have:

PORTNAME=    unit
PKGNAMEPREFIX= ${PYTHON_PKGNAMEPREFIX}
PKGNAMESUFFIX= -python

So the package names will end up being py27-unit-python and py36-unit-python. If you really want to repeat the fact that it is python, then do so, but I find it strange as it is purely artificial and superfluous.

osa added a comment.EditedDec 5 2017, 4:14 AM

Hi @lwhsu,

as far as I see the EMBED option hasn't been defined yet for
lang/php* modules, is it the blocker for new php*-unit modules?

Do we really need '-python' suffix for python-specific modules? I believe 'py*-' prefix is enough in this case.

lwhsu updated this revision to Diff 36227.Dec 5 2017, 8:05 AM

Rename www/py-unit-python to www/py-unit

lwhsu added a comment.Dec 5 2017, 8:09 AM
In D13227#279177, @osa wrote:

Hi @lwhsu,
as far as I see the EMBED option hasn't been defined yet for
lang/php* modules, is it the blocker for new php*-unit modules?

Not anymore, I committed the change to make it as default.
In fact, I got the approval from the lang/php* maintainers but thought I can commit it before this patch gets accepted.

Do we really need '-python' suffix for python-specific modules? I believe 'py*-' prefix is enough in this case.

My thought is in the previous, since you and @mat both think it's not necessary, I removed the suffix.

osa added a comment.Dec 9 2017, 4:46 PM

Hi @lwhsu,

so the final version of the patch looks good and I hope @mat has no more objection in this case as well, so I approve it.

lwhsu added a comment.Dec 9 2017, 4:56 PM

@osa , Thanks, hope adding these slave ports doesn't introduce too much trouble to you :-)

You can simply select "Accept Revision" in the "Add Action..." list at the bottom and then click "Submit" to explicitly accept the patch in this system. Though this is not necessary, just FYI.

I think @mat is also fine with it so I am going to commit this patch. Thanks for the help from both of you!

This revision was automatically updated to reflect the committed changes.
brnrd added a subscriber: brnrd.EditedDec 11 2017, 10:32 AM

This results in an error on my package builder:

[00:02:57] Warning: (www/unit-php71): [00:02:57] Error: Duplicated origin for unit-php71-0.2_1: www/unit-php71 AND www/unit-php56. Rerun with -v to see which ports are depending on these.

Version settings on another host

$ make -VDEFAULT_VERSIONS
apache=2.5 mysql=10.1m php=7.1 ruby=2.3 python=2.7

When building unit_php70

$ make -VPKGNAME
unit-php71-0.2_1
$ make build-depends-list
/usr/ports/ports-mgmt/pkg
/usr/ports/lang/php71

From configure output

Configuration summary:

  unit bin directory:           "/usr/local/bin"
  unit sbin directory:          "/usr/local/sbin"
  unit modules directory:       "/usr/local/libexec/unit/modules"
  unit state directory:         "/usr/local/libexec/unit"

  unit pid file:                "/var/run/unit/unit.pid"
  unit log file:                "/var/log/unit/unit.log"

  unit control API socket:      "unix:/var/run/unit/control.unit.sock"

  non-privileged user:          "nobody"
  non-privileged group:         ""

  IPv6 support:                 YES
  Unix domain sockets support:  YES
  debug logging:                NO

configuring PHP module
checking for PHP ... found
 + PHP version: 7.1.12
 + PHP SAPI: [cli embed fpm phpdbg cgi]
checking for PHP embed SAPI ... found
 + PHP module: php71.unit.so
===>  Building for unit-php71-0.2_1
lwhsu added a comment.Dec 11 2017, 1:32 PM

I guess this is because www/unit-php56 is using default PHP version (5.6) in the ports tree. I should explicitly set it. Sorry for the breakage. I will test the new patch and fix it.