configure: put lua-resty libs into a list, and make all operations on lua-resty opts use that list by simpl · Pull Request #362 · openresty/openresty

simpl · 2018-04-20T21:02:51Z

Reason for patch

The configure file now has quite a large number of included lua-resty-[name] libs, with all references to them statically included in the following places:

Configure option parsing (each --without-lua_resty_xxx option)
Including the make files (a static list)
Configure usage message

This is both more prone to errors, and is inefficient when adding new libs in the future.

What this patch does

Puts all the lua-resty-[name] libs into a list (underneath the list of modules)
Uses the list to generate a regex to test for --without-lua_resty_[name] configure options
Uses the list to add the lua-resty-[name] libs to the whole $make process
Uses the list to generate the relevant parts of the ./configure --help message
A few very minor edits that improve clarity / formatting consistency (that make no changes to the actions)

This makes the whole configure script a bit easier to read, makes it quicker to add new lua-resty-[name] libs to OpenResty, and reduces the possibility for bugs in future edits.

What else you might want to change

I've ordered the lua-resty-[name] modules alphabetically, because I think they're easier to read that way. Given that the modules are not listed alphabetically in the ./configure --help, you might want to :

Re-order the list of lua-resty-[name] libs
Modify the ./configure script to display the help with the --with-[name]_module and --without-[name]_module so they display alphabetically

I didn't want to presume your choice about this, but if you'd like me to do a quick patch to change that as well, I'm happy to do so.

… lua-resty opts use that list

simpl · 2018-04-20T21:52:00Z

Just to let you know, I double-checked to make sure that I hadn't forgotten any libs, and I tested with the following options :

    --without-lua_cjson
    --without-lua_redis_parser
    --without-lua_rds_parser
    --without-lua_resty_core
    --without-lua_resty_dns
    --without-lua_resty_limit_traffic
    --without-lua_resty_lock
    --without-lua_resty_lrucache
    --without-lua_resty_memcached
    --without-lua_resty_mysql
    --without-lua_resty_redis
    --without-lua_resty_string
    --without-lua_resty_upload
    --without-lua_resty_upstream_healthcheck
    --without-lua_resty_websocket

which then produced no lualib directory in $PREFIX folder. Obviously leaving these options off installs all the libs, so functionally there is no change to behaviour, just a tidy-up of the script.

… name contains an underscore

… DESTDIR was non-empty

agentzh

Please rebase to the latest master branch and make sure t/sanity.t test file is still passing (it's currently passing on the latest master). Thanks!

agentzh · 2018-04-24T23:35:21Z

 if ($platform ne 'msys') {
    push @make_install_cmds,
-        "ln -sf $ngx_sbin \$(DESTDIR)$prefix/bin/openresty";
+        "ln -sf \$(DESTDIR)$ngx_sbin \$(DESTDIR)$prefix/bin/openresty";


This is wrong. It will create a symlink pointing to the wrong location.

I guess it depends on where you want to have it link to, but if you have DESTDIR defined, then the symlink will link to the location without $(DESTDIR) added. All other files are installed with $(DESTDIR) prefixed, so this probably isn't what you want. Nginx is installed to the prefixed location. Adding $(DESTDIR) to the $ngx_sbin path makes it point to the nginx binary in the same root folder in all situations.

There is also another problem if you have DESTDIR set. That is that because the rpath and link locations are defined statically, nginx doesn't find libluajit, unless you specify it in the environment. IMHO it would be better to have relative symlinks and relative paths set. I was going to make the necessary changes to make them relative paths, so everything worked as expected if you move the output dir and submit them too. It makes more sense to me to be able to move the whole folder and for everything still to work, but let me know if that's not what you want.

@mclyne The DESTDIR variable is usually used for rpm/deb packaging tools to install the software to a temporary location other than under the real root directory right before generating an rpm or deb package. So it is not meant to be the final location for the installed packages. That's why I said if you add $(DESTDIR) to the symlink target location, it would be completely wrong (the DESTDIR location won't even exist in the final deployment machine).

@mclyne Seems like you confused DESTDIR with PREFIX. They are for completely different purposes.

@mclyne So your change above would lead to broken symlinks installed to the final machines after installing the deb/rpm packages.

I wasn't confusing DESTDIR with PREFIX, however I wasn't thinking of it being used in that way. Thanks for the explanation.

agentzh · 2018-04-24T23:36:23Z

    }

-    # configure opm:
+    # configure opm


Why this change?

Consistency with other comments. It was sort of an accidental inclusion. Same for below. Most comments don't have colons - though actually they're not totally consistent in the formatting.

agentzh · 2018-04-24T23:36:29Z

    }

-    # configure resty-cli:
+    # configure resty-cli


Rebase

configure: put lua-resty libs into a list, and make all operations on…

22db531

… lua-resty opts use that list

simpl force-pushed the master branch from bbbd855 to 51f7456 Compare April 20, 2018 22:22

configure: help now displays hyphens for names lua-resty-[name] where…

7cdd3da

… name contains an underscore

simpl force-pushed the master branch from 51f7456 to 7cdd3da Compare April 20, 2018 22:24

bugfix: configure: symlink to $ngx_sbin resolved to wrong location if…

62cb871

… DESTDIR was non-empty

agentzh requested changes Apr 24, 2018

View reviewed changes

Merge pull request #1 from openresty/master

f645a6f

Rebase

agentzh force-pushed the master branch from 5618ea0 to bf2e569 Compare May 8, 2019 21:47

Navigation Menu

configure: put lua-resty libs into a list, and make all operations on lua-resty opts use that list - #362

configure: put lua-resty libs into a list, and make all operations on lua-resty opts use that list#362
simpl wants to merge 4 commits into
openresty:masterfrom
simpl:master

simpl commented Apr 20, 2018

Uh oh!

simpl commented Apr 20, 2018

Uh oh!

agentzh left a comment

Uh oh!

agentzh Apr 24, 2018

Uh oh!

simpl Apr 25, 2018

Uh oh!

agentzh Apr 25, 2018 •

edited

Loading

Uh oh!

agentzh Apr 25, 2018

Uh oh!

agentzh Apr 25, 2018 •

edited

Loading

Uh oh!

simpl Apr 25, 2018

Uh oh!

agentzh Apr 24, 2018

Uh oh!

simpl Apr 24, 2018 •

edited

Loading

Uh oh!

agentzh Apr 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

simpl commented Apr 20, 2018

Reason for patch

What this patch does

What else you might want to change

Uh oh!

simpl commented Apr 20, 2018

Uh oh!

agentzh left a comment

Choose a reason for hiding this comment

Uh oh!

agentzh Apr 24, 2018

Choose a reason for hiding this comment

Uh oh!

simpl Apr 25, 2018

Choose a reason for hiding this comment

Uh oh!

agentzh Apr 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agentzh Apr 25, 2018

Choose a reason for hiding this comment

Uh oh!

agentzh Apr 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simpl Apr 25, 2018

Choose a reason for hiding this comment

Uh oh!

agentzh Apr 24, 2018

Choose a reason for hiding this comment

Uh oh!

simpl Apr 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agentzh Apr 24, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

agentzh Apr 25, 2018 •

edited

Loading

agentzh Apr 25, 2018 •

edited

Loading

simpl Apr 24, 2018 •

edited

Loading