[NEW PORT] www/angie: patch also includes new related ports for dynamic modules

[fuz]

Initial review of the port. Sorry that it took so long. I have not yet done a build test.

www/angie-module-geoip2/Makefile

9 ↗

(On Diff #121200)

For slave ports, please stick with the MASTERDIR mechanism as outlined in § 5.11 Porter's Handbook. The master makefile include should be the last line in the file and MASTERDIR should be set.

17 ↗

(On Diff #121200)

This of course doesn't work when you include the master makefile after the target is given. There's an easy workaround though: use the target do-install-DEBUG-on to install the debug files. This target is only run if the DEBUG option is set. I wonder if it might be possible to streamline the installation process further; it seems like it's basically the same for all slave ports. Perhaps you can simply set some variables in the slave port and have the same rule for all of them in the master port?

www/angie-module-geoip2/pkg-descr

2 ↗

(On Diff #121200)

pkg-descr should be at least three lines.

www/angie-module-geoip2/pkg-plist

5 ↗

(On Diff #121200)

When you use the MASTERDIR mechanism, you'll have to teach the Makefile where to find the right plist for the slave port. Check devel/schilybase for inspirations. There I have

PLIST= ${.CURDIR}/pkg-plist

to grap the plist from the slave directory.

www/angie/bsd.angie.mk

97 ↗

(On Diff #121200)

Here too you might be able to use targets like post-patch-DEBUG-on and so on.

[oleg_mamontov.net]

Sorry for the delay, this time from my side.
From the Porters Handbook it seems that MASTERDIR is recommended for building slightly different versions of packages but in this particular case we have a bunch of almost independent ports that still share a significant amount of parts.
I see many similar approaches in the tree, please take a look at www/twiki* for example.
In my opinion this seems more natural and convenient than using MASTERDIR for this use case.
Please let me know what do you think? If you are still sure that MASTERDIR is the only possible viable solution, I'll take this approach.

As for the rest of little things (like too short descriptions and so on), I will fix them all as soon as we agree on the main issue.
Thank you.

[fuz]

I strongly recommend that you stick to the MASTERDIR scheme unless there is something that cannot easily be done with it. From my previous review I haven't seen anything that would be difficult to achieve with MASTERDIR. Is there anything of concern?

The twiki family of ports is very old and hasn't had any updates since 2018 (there are some newer commits, but these are general maintenance commits applying to all ports). I don't think it is representative of how things should be done.

[oleg_mamontov.net]

Thank you for suggestion.
I've re-worked patch with MASTERDIR approach and shortened the patch to only angie and angie-module-geoip2 ports for ease of review process. If we can agree on this method, I'm ready to introduce all the others.
Thank you in advance!

[fuz]

Patch set looks good and can be committed with small changes. Over in PR 270933, @wen has given approval to override his port with this one.

Here are some issues you may want to check:

• the port may become a bit simpler if you moved the common Makefile stuff into a file like www/angie/Makefile.master included by all ports as the master makefile, including by www/angie/Makefile. This should remove most of the .if ${PORTNAME} == angie stuff. You can check devel/schilybase and its slave ports (e.g. sysutils/cdrtools) for inspiration. The current approach should be fine, too.
• ensure that the package version is newer than that of the port that is currently in the tree. If not, bump PORTREVISION and PORTEPOCH as needed.
• if there is a test suite, see if you can hook it up into the ports framework (with TEST_TARGET or do-test or similar)
• do not hardcode /usr/bin/install. Instead, use the ${INSTALL} macro.
• do not use the naked cp command. Use ${CP} instead.

A build test succeeds but yields a number of warnings. Please check these and rework the patch:

====> Running Q/A tests (stage-qa)
Warning: Bad symlink '/usr/local/etc/angie/modules' pointing to an absolute pathname '/usr/local/libexec/angie'
Warning: 'sbin/angie-debug' is not stripped consider trying INSTALL_TARGET=install-strip or using ${STRIP_CMD}
====> Running Q/A tests (stage-qa)
Warning: 'libexec/angie/ngx_http_geoip2_module-debug.so' is not stripped consider trying INSTALL_TARGET=install-strip or using ${STRIP_CMD}
Warning: 'libexec/angie/ngx_stream_geoip2_module-debug.so' is not stripped consider trying INSTALL_TARGET=install-strip or using ${STRIP_CMD}
Warning: you may not need USES=ssl

Stripping the binary is not mandatory. You may leave the binary unstripped, especially if there is a good reason for doing so. For the link issue, you may use the ${RLN} command as documented in Porter's handbook.

Once these issues are resolved, I can commit this patch, replacing @wen's port. We can then continue with the remaining modules.

www/angie/Makefile

2

This should probably be DISTVERSION unless you know that PORTVERSION doesn't work.

[oleg_mamontov.net]

Hi
Please find my comments below in-line:

The port may become a bit simpler if you moved the common Makefile stuff into a file like www/angie/Makefile.master included by all ports as the master makefile, including by www/angie/Makefile. This should remove most of the .if ${PORTNAME} == angie stuff. You can check devel/schilybase and its slave ports (e.g. sysutils/cdrtools) for inspiration. The current approach should be fine, too.

I've tried this approach but wasn't really satisffied with the results.
I have to .include <bsd.port.options.mk> before the first target because a number of variables should be already defined at that moment.
Thus, .include "Makefile.master" in the last line seems to be too late.
Also, as for me, such separation can even worsen the readability against current solution

Ensure that the package version is newer than that of the port that is currently in the tree. If not, bump PORTREVISION and PORTEPOCH as needed.

Fixed with "PORTREVISION= 1", thanks.

If there is a test suite, see if you can hook it up into the ports framework (with TEST_TARGET or do-test or similar)

Indeed, there is a test suite but it's quite complex to be injected on package building stage.
For sure, we have CI/CD pipelines running internally on a matrix of platforms/architectures before each release.

Do not hardcode /usr/bin/install. Instead, use the ${INSTALL} macro.

As far as I can see, there is no top-level "INSTALL" macro. It's only defined in standard do-configure: target in Mk/bsd.port.mk.
That was a reason to define it the same way. Please correct me if this understanding is wrong.

Do not use the naked cp command. Use ${CP} instead.

Fixed, thanks.

Stripping the binary is not mandatory. You may leave the binary unstripped, especially if there is a good reason for doing so.

You are correct, regular binaries are accompanied with non-stripped debug versions on purpose.
Rarely but, third-party module may crash the whole worker process so it's very handy to have a readable core dump that allows to diagnose and exclude the culprit from the configuration.

For the link issue, you may use the ${RLN} command as documented in Porter's handbook.

Thanks for the suggestion, I wasn't aware of this useful macro.

I've just updated the proposal with above corrections.

[fuz]

LGTM. Will commit with my next batch.