Page MenuHomeFreeBSD

www/nginx: make easier to read and maintain
ClosedPublic

Authored by joneum on Mar 20 2018, 6:44 PM.

Details

Summary

www/nginx: make easier to read and maintain:

  • Objectives:
      • make easier to read and maintain
      • Module config in a single section
      • use OPTIONS framwework where possible
    • Add options groups for mail and http
    • Use options groups to set _IMPLIES instead of large .if HTTP and MAIL blocks
    • Rename all _VERSION vars to align with the OPTION name
    • Add DEVEL_KIT option so we can automate
    • Separate bundled and 3rd party modules
    • Order options alphabetically
    • Pass openssl dir to configure
    • Separate external modules in separate makefile

www/nginx is buildable again with this:

  • Fix sub-paths for DSO_EXTMODS introducing ${WRKSRC_mod}
    • Be smarter about the if ${PORT_OPTIONS:MDSO}
    • move post-install-OPT-on to where possible
    • Add some OPT_NGINX_VER if used multiple times
Test Plan

i will add @robak to this review. He is Maintainer from the Salve Ports for nginx.

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

joneum created this revision.Mar 20 2018, 6:44 PM
mat added a comment.Mar 21 2018, 1:51 PM

While you are at it, replace all the foo_GH_* with foo_GH_TUPLE, for all the extensions from github, it will replace 4 lines with only 1.

brnrd added a comment.Mar 21 2018, 4:52 PM

Thanks for posting it here instead! I started this because every time I needed to look at nginx Makefile it made my head hurt...

In D14773#310715, @mat wrote:

While you are at it, replace all the foo_GH_* with foo_GH_TUPLE, for all the extensions from github, it will replace 4 lines with only 1.

Done in my local repo, will update this review.

Haven't collapsed all as I expect intent behind the separate line with the version numbers (could even push that to an included file). Putting the version in the tuple reduces some options by another 33.3% in size. 2-line modules, MODNAME_GH_TUPLE and MODNAME_CONFIGURE_ION are all that's left then.

Could actually use a bit of help here @mat , I'm struggling with variable expansion here. The DSO_EXTMODS loop in the main Makefile doesn't work as EXTMODS isn't declared yet after bsd.port.options.mk. There's some evidence of using := in the Makefile.extmods file but that didn't work and neither did using bsd.port.pre.mk in stead of bsd.port.options.mk.

brnrd edited the summary of this revision. (Show Details)Mar 21 2018, 4:53 PM
brnrd edited the test plan for this revision. (Show Details)
brnrd updated this revision to Diff 40561.Mar 21 2018, 6:53 PM

Fold _GH_* params to _GH_TUPLE

Remove _NGINX_VERSION vars too

brnrd edited the summary of this revision. (Show Details)Mar 21 2018, 7:02 PM
mat added a comment.EditedMar 26 2018, 3:06 PM

The DSO_EXTMODS does not work because of how opt_VARS does. It interpolates whatever you pass to it, which is bad because at the time when b.options.m.k is evaluated, the GitHub code has not ran so all the WRKSRC_* variables are unset.

What you should do is fill in DSO_EXTMODS with either WRKSRC_x (not the variable, the litteral) or just the x part, and then simply do:

.for modpath in ${DSO_EXTMODS}
.  if ${PORT_OPTIONS:MDSO}
CONF_ARGS+=     --add-dynamic-module=${WRKSRC_${modpath}}
.  else
CONF_ARGS+=     --add-module=${WRKSRC_${modpath}}
.  endif
.endfor
mat added a comment.Mar 26 2018, 3:07 PM

Also, do not use := as you don't seem to understand what it does ;-)

www/nginx/Makefile
165 ↗(On Diff #40561)

+=

186 ↗(On Diff #40561)

+=

brnrd updated this revision to Diff 40759.Mar 26 2018, 7:34 PM

Fix configure args for EXTMODS

  • Split DSO_EXTMODS and DSO_EXTDIRS
  • Make the ext-mod --add-module stuff work
  • Use more target-OPT-on:
  • Sort target-OPT-on alphabetically

TODO: Fix objs/Makefile pcre bug

brnrd updated this revision to Diff 40762.Mar 26 2018, 8:04 PM

Fix do-build (this is not autoconf!)

  • Remove --with-openssl
  • Remove --with-builddir

Now actually builds. Next challenge: pkg-plist

brnrd updated this revision to Diff 40766.Mar 26 2018, 8:44 PM

Fixups / typos

  • Fix lingering ${MOD_VERSION} refs in Makefile.extmod
  • Convert post-patch-MOD-on: targets to use ${WRKSRC_mod}

For nginx-full, make patch now completes OK

brnrd updated this revision to Diff 40768.Mar 26 2018, 9:12 PM

Move some modules to dynamically loadable

  • As per the module's config file
brnrd added a comment.EditedMar 26 2018, 9:16 PM
In D14773#312227, @mat wrote:

Also, do not use := as you don't seem to understand what it does ;-)

I am learning all the way 😄 and have better understanding now.

In D14773#312224, @mat wrote:

The DSO_EXTMODS does not work because of how opt_VARS does. It interpolates whatever you pass to it, which is bad because at the time when b.options.m.k is evaluated, the GitHub code has not ran so all the WRKSRC_* variables are unset.
What you should do is fill in DSO_EXTMODS with either WRKSRC_x (not the variable, the litteral) or just the x part, and then simply do:

.for modpath in ${DSO_EXTMODS}
.  if ${PORT_OPTIONS:MDSO}
CONF_ARGS+=     --add-dynamic-module=${WRKSRC_${modpath}}
.  else
CONF_ARGS+=     --add-module=${WRKSRC_${modpath}}
.  endif
.endfor

Exactly! That's what I've figured out independently... Saw your remarks too late 😢

@mat How do you like the looks of the port now? Better?

Asked on #bsdports as well, should the dynamic plist in post-install not be migrated to static pkg-plist?

%%DSO%%%%STREAM%%%%MODULE%%libexec/nginx/ngx_stream_mod.so
joneum updated this revision to Diff 40805.Mar 27 2018, 4:53 PM

Final Version

brnrd updated this revision to Diff 40813.Mar 27 2018, 6:40 PM

www/nginx is buildable again with this

  • Fix sub-paths for DSO_EXTMODS introducing ${WRKSRC_mod}
  • Be smarter about the if ${PORT_OPTIONS:MDSO} check
  • move post-install-OPT-on to where possible
  • Add some OPT_NGINX_VER if used multiple times

Open: Passenger, Clojure, AJP options

joneum edited the summary of this revision. (Show Details)Mar 27 2018, 7:05 PM
joneum edited the summary of this revision. (Show Details)
joneum edited the summary of this revision. (Show Details)
joneum updated this revision to Diff 40817.Mar 27 2018, 7:07 PM
joneum edited the summary of this revision. (Show Details)

after some fixes, here are the finaly version

joneum edited the test plan for this revision. (Show Details)Mar 27 2018, 7:35 PM
joneum added a reviewer: robak.
joneum added a subscriber: robak.
brnrd added inline comments.Mar 27 2018, 8:04 PM
www/nginx/Makefile
165 ↗(On Diff #40561)

Fixed locally, thanks for spotting!

186 ↗(On Diff #40561)

Fixed locally

brnrd accepted this revision.Mar 27 2018, 8:05 PM

Tested building nginx, nginx-lite, nginx-full and nginx-naxsi and these are now all OK.

Reduced the lines of Makefile from ca 1400 to 750...

This revision is now accepted and ready to land.Mar 27 2018, 8:05 PM
joneum updated this revision to Diff 40828.Mar 28 2018, 5:04 AM

fix Problem with "pkg-static: Unable to access file"

This revision now requires review to proceed.Mar 28 2018, 5:04 AM
mat added inline comments.Mar 28 2018, 9:04 AM
www/nginx/Makefile
192 ↗(On Diff #40828)

?

201–203 ↗(On Diff #40828)

Use an OPTIONS_MULTI for that.

brnrd added inline comments.Mar 28 2018, 10:40 AM
www/nginx/Makefile
192 ↗(On Diff #40828)

removed from local version.

Comments need to be removed still. Also see top of Makefile. That is not supposed to be committed.

201–203 ↗(On Diff #40828)

That would remove the HTTP option from the HTTPGRP and the MAIL option from MAILGRP. Can be done but that puts the MAIL and HTTP options out of context in make config

All http modules have an //OPT//_IMPLIES= HTTP and all mail modules imply MAIL (lines 97 and 101).
I can't imagine anyone running into this error as it basically means that all modules options (including HTTP and MAIL) have been disabled and the user is requesting an nginx that wouldn't actually do anything. Nginx' configure would throw an error as well.

This revision was not accepted when it landed; it landed in state Needs Review.Mar 28 2018, 3:02 PM
This revision was automatically updated to reflect the committed changes.