Page MenuHomeFreeBSD

www/nginx: make easier to read and maintain
ClosedPublic

Authored by joneum on Mar 20 2018, 6:44 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Mar 22, 3:47 PM
Unknown Object (File)
Thu, Mar 7, 2:02 PM
Unknown Object (File)
Dec 23 2023, 12:34 AM
Unknown Object (File)
Dec 7 2023, 9:51 AM
Unknown Object (File)
Nov 24 2023, 7:39 PM
Unknown Object (File)
Nov 10 2023, 12:10 AM
Unknown Object (File)
Nov 8 2023, 1:02 PM
Unknown Object (File)
Nov 8 2023, 3:59 AM
Subscribers

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
No Lint Coverage
Unit
No Test Coverage
Build Status
Buildable 15809
Build 15820: arc lint + arc unit

Event Timeline

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.

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 test plan for this revision. (Show Details)

Fold _GH_* params to _GH_TUPLE

Remove _NGINX_VERSION vars too

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

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

www/nginx/Makefile
250

+=

271

+=

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

Fix do-build (this is not autoconf!)

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

Now actually builds. Next challenge: pkg-plist

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

Move some modules to dynamically loadable

  • As per the module's config file
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

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)
joneum edited the summary of this revision. (Show Details)
joneum edited the summary of this revision. (Show Details)

after some fixes, here are the finaly version

joneum added a reviewer: robak.
joneum added a subscriber: robak.
www/nginx/Makefile
250

Fixed locally, thanks for spotting!

271

Fixed locally

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

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

This revision now requires review to proceed.Mar 28 2018, 5:04 AM
www/nginx/Makefile
218–234

?

236–239

Use an OPTIONS_MULTI for that.

www/nginx/Makefile
218–234

removed from local version.

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

236–239

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.