Page MenuHomeFreeBSD

[NEW PORT] devel/oniguruma6: Oniguruma library version 6
ClosedPublic

Authored by lifanov on Dec 20 2016, 7:29 PM.

Details

Summary
  • new port: devel/oniguruma

Oniguruma is a regular expressions library. The characteristics of this
library is that different character encoding for every regular
expression object can be specified.

PR: 212715
Submitted by: Yuri Victorovich <yuri@rawbw.com>, Robert Zelaya <rob@theseusnetworking.com> (original version)
Reviewed by: matthew, mat
Approved by: matthew (mentor)
Differential Revision: https://reviews.freebsd.org/D8871

Test Plan

poudriere testport : OK
portlint -a : OK

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

lifanov retitled this revision from to [NEW PORT] devel/oniguruma6: Oniguruma library version 6.
lifanov updated this object.
lifanov edited the test plan for this revision. (Show Details)
lifanov added a reviewer: matthew.
lifanov added a subscriber: koobs.

rename to devel/onigurama, set Yuri as maintainer

Whoops, apparently arc scoops stuff up from parent directories too...

devel/Makefile
1710 ↗(On Diff #23155)

You should explain why you're changing the existing naming convention in the summary -- and the summary should form the body of your commit message.

devel/oniguruma/Makefile
9 ↗(On Diff #23155)

As rob@theseusnetworking.com is the maintainer of the other two oniguruma ports and the originator of the PR to create this port, please make sure he agrees with handing over maintainership for this port.

10 ↗(On Diff #23155)

Why have you dropped DIST_SUBDIR here?

20 ↗(On Diff #23155)

Is this CONFLICTS setting correct? Given that the only versions in ports are oniguruma-4, -5 and this -6 probably not.

If possible, can you use CONFLICTS_INSTALL rather than CONFLICTS ? That requires that you can build this port when one of the conflicting ports is already installed. If building like that breaks, then just carry on using plain CONFLICTS.

devel/oniguruma/pkg-descr
13 ↗(On Diff #23155)

While attributing bits of work to their authors is generally a good thing, it's not what pkg-descr is for. This sort of thing can be included in the package documentation.

19 ↗(On Diff #23155)

You don't want to talk about change history / version dependent stuff in pkg-descr: this file is purely to tell the users *what* this port is, and the text should be independent of the precise version of the software.

26 ↗(On Diff #23155)

Similarly, you don't want to put this license stuff into pkg-descr -- it's already in the port Makefile.

portlint will moan at you if pkg-descr is too long -- that's more a guideline than a rigidly enforced rule though.

matthew edited edge metadata.

Builds fine.

As noted, the CONFLICTS setting looks non-sensical. That will need fixing in all of the devel/oniguruma* ports, not just this new port. You can fix that sort of ports infrastructure thing without asking the maintainer, but as you should be contacting him anyhow, it might be an idea to mention what you're going to do. (Some maintainers can get pretty possessive of *their* ports, but a little diplomacy goes a long way.)

The pkg-descr needs trimming down as indicated. For new ports, running 'portlint -a' is mandatory (even if they were copied from another port).

My other comments are on things you'll need to justify in your commit message. That's ideally a brief outline of what you changed and -- most importantly -- why.

The 'Summary' section in a review should contain pretty much the same stuff as you'ld put in the commit message

This revision now requires changes to proceed.Dec 21 2016, 12:04 AM
devel/Makefile
1710 ↗(On Diff #23155)

It should be added as oniguruma6, like the others.

devel/oniguruma/Makefile
9 ↗(On Diff #23155)

I am trying my best go get in touch with him.

10 ↗(On Diff #23155)

This isn't a ruby thing. It's a C library written in C and it's used by other projects, like PHP, jq, and mosh to name a few.

20 ↗(On Diff #23155)

CONFLICTS setting is not correct. I'll fix it

devel/Makefile
1710 ↗(On Diff #23155)

The volunteer maintainer for the new port (Yuri) would prefer it this way. If I hear from Robert, I will keep it oniguruma6. There are other examples in the ports tree with the bare and versioned ports.

lifanov edited edge metadata.

address feedback by matthew

lifanov edited the test plan for this revision. (Show Details)
lifanov edited edge metadata.
devel/Makefile
1710 ↗(On Diff #23155)

The original submitter put it there.

Also, the porter's handbook says that:

We reserve the right to modify the maintainer's submission to better match existing policies and style of the Ports Collection without explicit blessing from the submitter or the maintainer.

Put it in a versioned directory, it is the current trend for the ports tree :-)

Also, don't forget to add PKGNAMESUFFIX=6.

devel/oniguruma/Makefile
20 ↗(On Diff #23182)

The conflict is wrong, there are no ports named oniguruma-[45].*, there are, on the other hand, ports named oniguruma4-* or oniguruma5-*.

9 ↗(On Diff #23155)

I agree with matthew here, keep the original MAINTAINER.

whoops, fix CONFLICTS_INSTALLED as suggested by mat

reinstate maintainer from original submission

lifanov marked 4 inline comments as done.
matthew edited edge metadata.

This looks good to me.

This revision is now accepted and ready to land.Dec 22 2016, 3:08 PM

Also, don't forget to fix the CONFLICTS in devel/oniguruma4 and devel/oniguruma5.

This revision was automatically updated to reflect the committed changes.

Hi matthew, I haven't been able to get in touch with Robert. Can I just submit a review and do it?

Updating CONFLICTS? Yes. Go for it.