Page MenuHomeFreeBSD

lang/lua53 - Update to 5.3.5
ClosedPublic

Authored by russ.haley_gmail.com on Dec 30 2017, 7:09 AM.

Details

Summary

Hello,

This patch is for 5.3.5, the final release of Lua 5.3. The source code includes all patches so they have been removed from the port.

Changes to the port Makefile include:

  • Some cleanup of the variables
  • Changed to use the bsd target in the Lua Makefile.
  • Added new build options for Lua html documentation, editline/readline, and two Lua build in debugging options
  • I have done some investigation and -pthead is required to use libressl with luaossl (as per comment) so I've left that and -lm in the port.

Update 2018-07-17: -pthread has been removed. As follows:

The inclusion of -pthread and it's fix has been described by jbeich as such:
-pthread was inherited from
https://svnweb.freebsd.org/changeset/ports/324286
which was probably fixed by
https://svnweb.freebsd.org/changeset/base/276630

My only known use case for including -pthread was from section 1.2.3 of the luaossl manual
http://25thandclement.com/~william/projects/luaossl.pdf:

"Note that on some systems, such as NetBSD and FreeBSD, the loading application must be linked against pthreads (using lpthread or -pthread). It is not enough for the luaossl module to pull in the dependency at load time. In particular, if using the stock Lua interpreter, it must have been linked against pthreads at build time. Add the appropriate linker flag to MYLIBS in lua-5.2.x/src/Makefile."

The above issue seemingly fixed in my simple use cases evidenced in the Test Plan update on 2018-07-17.

Conclusion:
I conclude that it is safe to remove -pthread with two testing scenarios outstanding. My reasons for this conclusion are two:

  1. It is not part of the standard Makefile and GNU/Linux distros don't seem to use it either (checked Debian and Arch. I have seen the emails online of -pthread being removed from the Fedora build).
  2. There is evidence that none of the prior known issues are still applicable

The two doubts I still hold are building luaossl against libressl and the potential that advanced use cases of luaossl may still be an issue (please see my Test Plan for further information). My opinion is these edge cases should not be used to force the inclusion of -pthread and those using luaossl against libressl can build their own version.

Test Plan
  • Test on x86 and arm.
  • Test the removal of -prthread.
  • further pthread testing: Create a new TrueOS image (which uses libressl) and run a lua-http web client/server to test certificates (See Summary for information).
  • Update 2018-07-16:

Testing on amd64 ongoing.
Tested on Arm 11.2 and 12 Current (also ongoing).

Update 2018-07-17:
I have performed some testing with Lua and luaossl. I have compiled the interpreter both with and without -pthread and I was able to 'require()' luaossl. I checked the openssl version number and created a public key. I have recorded building and running luaossl without prthread here: https://pastebin.com/WD7uvUan

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

There are a very large number of changes, so older changes are hidden. Show Older Changes
mat added inline comments.Jul 14 2018, 12:44 PM
Makefile
7 ↗(On Diff #45273)

This should be resetted when updating the version.

70 ↗(On Diff #45273)

Why?

Makefile
48 ↗(On Diff #45273)

No. Linking lua53 with pthread might be defensible (though I personally don't think it is), but linking liblua.so against pthread is not on.

Makefile
48 ↗(On Diff #45273)

I was unable to find a definitive test to indicate if Lua requires linking pthread. As I had seen mention of it in luaossl, I built Lua with and without linking to pthread and created a public key. In both tests I "required" luaossl and successfully created a public key using 1.0.2k.

I emailed the luaossl maintainer (actually, I emailed the author wahern who directed me to the current maintainer duarnimator). Duarnimator indicated that openssl pre 1.1.0 needs pthreads and he pointed me to the sources:
https://github.com/wahern/luaossl/blob/357a7f6da9e6ae442b3c3b9e43396b3cf2e1a91d/src/openssl.c#L11127

At this juncture I have assumed my tests were faulty and added -pthread back in. If you can indicate some tests I could try on any package that would prove this one way or the other (or confirm that my original test was valid) I would like to remove it entirely.

To keep all the information together, here is the previous comment on that topic. As per an email with Jan Beich:

-pthread was inherited from
https://svnweb.freebsd.org/changeset/ports/324286
which was probably fixed by
https://svnweb.freebsd.org/changeset/base/276630

70 ↗(On Diff #45273)

I'm not sure I understand the question @mat?

I'll assume the question is "Why use variables" and say, I was trying to remove the hard coded 'lua' and 'lua53' strings to make it more consistent and usable for the upcoming 5.4 port. To be more accurate, this is a revision of the 5.4 port Makefile 'backported' to 5.3

Makefile
48 ↗(On Diff #45273)

For reference on the luaossl requirement, see http://25thandclement.com/~william/projects/luaossl.pdf section 1.2.3

@andrew_tao173.riddles.org.uk, I am very grateful for your tutelage and contributions. Do you want to see the readline options from D14709 left in? I had two thoughts: 1) anyone who doesn't want readline support is probably building Lua themselves and 2) with the freebsd target working, we could create an option for the generic target, that does not include readline.

I personally don't feel the extra complexity of the options adds value since the source Makefile is patched to use editline.

russ.haley_gmail.com retitled this revision from Lua53 revision 3 to Lua53 - Update to 5.3.5.Jul 14 2018, 6:15 PM
Makefile
47 ↗(On Diff #45297)

I'm struggling to understand if/why libmath is required for the shared object at all but I don't currently have any code that uses liblua.so. I'll postulate that we should do a second exp run with -lm removed to see if anything breaks?

russ.haley_gmail.com marked 2 inline comments as done.Jul 14 2018, 6:21 PM

@andrew_tao173.riddles.org.uk, I am very grateful for your tutelage and contributions. Do you want to see the readline options from D14709 left in? I had two thoughts: 1) anyone who doesn't want readline support is probably building Lua themselves and 2) with the freebsd target working, we could create an option for the generic target, that does not include readline.

I personally don't feel the extra complexity of the options adds value since the source Makefile is patched to use editline.

I have been working on a new version of the 5.4 port (since the need for NILINTABLE went away, and I wanted to experiment with other options).

Probably not worth adding options just for readline, but what about installing the HTML docs? I think that would be a good option (or indeed a default).

Makefile
47 ↗(On Diff #45297)

Lua calls functions from libm. If you try and load liblua.so into a main program that doesn't export the libm functions, it'll fail unless liblua.so pulls in libm itself. So it's always correct for liblua.so to be linked against libm.

48 ↗(On Diff #45273)

I think you're missing the point about pthreads.

Lua itself has no interaction with threads at all (unless you explicitly modify it to lock the interpreter, which the port doesn't do). Therefore, it's inappropriate for lublua.so (which will never call any pthread function) to force libthr into the process. (Remember that liblua.so might be being loaded because of a reference from a dlopen'd module - this is actually not uncommon)

The bug fix you reference (base/276630) is specifically about loading libthr via dlopen, so without that fix, linking lublua.so against libthr actually makes things worse, since any dynamic load of a lua interpreter would trigger the bug even if no pthread-using module was ever used from Lua.

If some extension that you want to load from Lua needs to pull in pthreads, then there are two possibilities: either it's enough for the extension .so to reference libthr to pull it in, or if there's an unfixed bug of the same kind as base/276630, then it's not enough for liblua.so to reference libthr, but rather the main executable must do so.

@andrew_tao173.riddles.org.uk, I am very grateful for your tutelage and contributions. Do you want to see the readline options from D14709 left in? I had two thoughts: 1) anyone who doesn't want readline support is probably building Lua themselves and 2) with the freebsd target working, we could create an option for the generic target, that does not include readline.

I personally don't feel the extra complexity of the options adds value since the source Makefile is patched to use editline.

I have been working on a new version of the 5.4 port (since the need for NILINTABLE went away, and I wanted to experiment with other options).

Probably not worth adding options just for readline, but what about installing the HTML docs? I think that would be a good option (or indeed a default).

I think it would be an excellent default.

Probably not worth adding options just for readline, but what about installing the HTML docs? I think that would be a good option (or indeed a default).

I think it would be an excellent default.

I updated D14709 with a new diff, you can see what I did with the docs there.

Makefile
70 ↗(On Diff #45273)

the problem here is that soname= and LUA_LIB_SHARED don't match if PORTNAME is changed. Either both names should be based on PORTNAME or neither should.

Corrected so name variables. Thanks @andrew_tao173.riddles.org.uk

russ.haley_gmail.com marked 5 inline comments as done.Jul 14 2018, 9:56 PM
russ.haley_gmail.com updated this revision to Diff 45308.EditedJul 15 2018, 2:10 AM

Backported D14709. The Lua 5.3 and 5.4 ports are now identical except version difference and the patch-src__Makefile is slightly different (to be reviewed).

russ.haley_gmail.com edited the test plan for this revision. (Show Details)Jul 15 2018, 2:16 AM
Makefile
124 ↗(On Diff #45308)

Wrong value for soname= there. LUA_LUB_SHARED is liblua-5.3.so, but soname= needs to be lua-5.3.so without the 'lib' prefix.

Makefile
124 ↗(On Diff #45308)

uh, I mean soname needs to be lua-5.3 without either the 'lib' prefix or the '.so' suffix.

Corrected soname value.

russ.haley_gmail.com marked 2 inline comments as done.Jul 15 2018, 2:45 AM
mat added inline comments.Jul 16 2018, 7:35 AM
Makefile
3 ↗(On Diff #45309)

Please, DO NOT CHANGE the header.

7 ↗(On Diff #45309)

This was not done. Please remove PORTREVISION, or set it to 0.

70 ↗(On Diff #45273)

Yes, why remove lua? This will never change, so, there is no point of having it as a variable. (Well, unless the language is renamed, but I don't see that happening.)

russ.haley_gmail.com edited the test plan for this revision. (Show Details)

Cleanup mistake. Lua counts from 1, ports revisions do not. :)

Makefile
70 ↗(On Diff #45273)

Yes, why remove lua? This will never change, so, there is no point of having it as a variable. (Well, unless the language is renamed, but I don't see that happening.)

I suppose the reason is two fold: 1) I'm learning as I go and it seemed like something to clean up and 2) my "OCD" made me cringe when I looked at it. I can change it back if it offends, as I am largely indifferent.

mat added inline comments.Jul 17 2018, 6:46 AM
Makefile
70 ↗(On Diff #45273)

It would be better to keep lua as it was before, using a 11 characters of variable for a 3 letter constant is a bit overboard.

Changed no name as per comment from @mat.

Makefile
70 ↗(On Diff #45273)

I am grateful for you taking the time to review my work. Please let me know what else you wish to see amended.

russ.haley_gmail.com edited the summary of this revision. (Show Details)Jul 17 2018, 7:34 PM
russ.haley_gmail.com edited the test plan for this revision. (Show Details)
mat added inline comments.Jul 18 2018, 7:19 AM
Makefile
1–3 ↗(On Diff #45423)

Please restore the header.

13 ↗(On Diff #45423)

Most scripting languages are embeddables, is lua not compilable any more ?

56 ↗(On Diff #45423)

Why +=?

linimon retitled this revision from Lua53 - Update to 5.3.5 to lang/lua53 - Update to 5.3.5.Jul 18 2018, 12:52 PM
Makefile
13 ↗(On Diff #45423)

What does "compilable" mean? There is a bytecode compiler, which is useful for syntax checking and faster code loading, but has no effect whatsoever on execution speed (since code loaded as text is compiled to the same bytecode before execution anyway). Bytecode compilation is also of limited usefulness since the bytecode is version and platform dependent.

I think most people would interpret "compilable" as implying faster execution, or execution of machine instructions rather than interpreted code, which is not the case here.

56 ↗(On Diff #45423)

I probably cargo-culted that in from some example when adding options to the lua54 port, and russ got it from me. No reason not to change it to just =

files/patch-src__Makefile
63 ↗(On Diff #45423)

This isn't actually needed with the change to use the "bsd" target.

Makefile
13 ↗(On Diff #45423)

@andrew_tao173.riddles.org.uk is correct, that's why I changed it. I finally decided on the current phrasing because 'compilable' is not in the official description and I was too cowardly to change it entirely. I will change it to the official description on Lua.org:

Lua is a powerful, efficient, lightweight, embeddable scripting language.

  • Updated according to comments. Thanks @mat.
  • Updated port description to quote from the Lua.org website. Now the same as lua-5.3.pc.in.
  • Removed extraneous patch of freebsd target in src/Makefile. Thanks @andrew_tao173.riddles.org.uk
russ.haley_gmail.com edited the summary of this revision. (Show Details)Jul 19 2018, 12:10 AM
mat added inline comments.Jul 19 2018, 7:47 AM
Makefile
67–78 ↗(On Diff #45503)

extra white space before =.

82–96 ↗(On Diff #45503)

missing tab after =.

102 ↗(On Diff #45503)

This is not used anywhere, what is it for?

Makefile
102 ↗(On Diff #45503)

It's the only way to stop bsd.port.mk from adding -fno-strict-aliasing to CFLAGS. Lua does not require this flag.

Makefile
76 ↗(On Diff #45503)

Well, this sucks: qa.sh doesn't allow this and demands that you use the port instead.

The problem with using the port libedit is that it links against the wrong libncurses, leading to compatibility problems when you try and use the lua curses module. Same problem exists with readline. I was hoping to avoid that problem this way, but if linking to base libedit isn't an option then we're going to need a much more involved solution, such as installing an extra lua binary without editing support.

New version to fix the libedit issue.

This one passes both portlint and qa.sh. Changes:

  • Add new patch for src/lua.c to dynamically load libedit, using RTLD_LOCAL, so that libedit's dependencies can't mess up loading of Lua modules. The old options for linking libedit or readline normally are still present, but not the default.
  • Backpatch from 5.4 the setting of rl_readline_name and rl_inhibit_completion, which are a notable usability enhancement
  • use DISTVERSION rather than PORTVERSION (consistency with 5.4 port)
  • editorialize the COMMENT to satisfy portlint; it's still the basic text from the lua.org website, but matches the style of other COMMENT fields
  • disuse PORTNAME in almost all of the remaining places (no point using it for some vars but not others; but I left it in DOCSDIR because I think it's cleaner that way)
  • ensure the .so gets linked with -lm as an explicit dependency
  • make whitespace and tabulation more consistent

I have tested that these changes resolve the lcurses conflict even when working interactively.

mat added inline comments.Jul 20 2018, 11:40 AM
Makefile
102 ↗(On Diff #45503)

But is it broken with it?

Makefile
102 ↗(On Diff #45503)

-fno-strict-aliasing only reduces optimization, so absent compiler bugs it can't "break" anything. However since Lua is an actively maintained piece of code which is tested with modern compilers, for which performance is a factor, I think it best to follow the upstream's selection of compile options in the absence of an overriding reason to do otherwise.

Makefile
31 ↗(On Diff #45566)

Ken Sasaki (lua-stdlib-normalize / lua-stdlib-debug maintainer) suggested in a bugzilla thread that perhaps modules should be installing docs under doc/lua5N/port-name, so perhaps we should consider installing the docs for Lua proper under doc/lua5N/lua to allow that without creating a mess?

150 ↗(On Diff #45566)

make check-orphans doesn't like this, I'll fix in the next patch

Fixes:

  • change DOCSDIR to lua53/lua so that lua53/${PORTNAME} could be used for lua module ports
  • reorganize DOCS install so that check-plist doesn't complain

So are we done here? I have nothing more to add.

So are we done here? I have nothing more to add.

I'm happy with it.

dbn accepted this revision.Jul 28 2018, 5:13 AM

I cannot comment on the specifics of lua (i.e. do these changes work), I've never used lua and haven't tested it. However, I am happy with the above changes with the following comments:

  1. I assume the indentation is correct, despite it not rendering correctly in Chrome
  2. Please try to upstream all patches (and well done on doing that for the previous version)

@jbeich: your thoughts?

And, who will commit this change?

This revision is now accepted and ready to land.Jul 28 2018, 5:13 AM
mat added inline comments.Jul 30 2018, 9:57 AM
Makefile
89–90 ↗(On Diff #45744)

To silence what exactly?

Do not add inactive code because a third party lint tool gives you warnings.

Also, what happens if libedit or libreadline is installed, and one selects this option?

154–157 ↗(On Diff #45744)

Any reason not to simply COPYTREE_SHARE all that directory in DOCSDIR?

Makefile
89–90 ↗(On Diff #45744)

portlint, which is prominently mentioned in the porters manual, counts as a third-party tool?

Without that line or something logically equivalent, portlint reports:

WARN: Makefile: EDITNONE is listed in OPTIONS_DEFINE, but no PORT_OPTIONS:MEDITNONE appears.

Obviously that can just be ignored, which I did for a while, but I figured that it cost nothing to suppress the warning.

If one selects EDITNONE, then regardless of what is or is not installed, the lua53 binary will not do command-line editing, when running interactively it'll just fgets from stdin. I thought the description of that was clear enough. No dependency on libedit or readline will be generated and no library other than libc and libm will be linked.

154–157 ↗(On Diff #45744)

BUILD_WRKDOC also has the lua.1 and luac.1 files in it that are installed as manpages rather than docs.

In a previous version I had it copying those files anyway, but just not including them in PORTDOCS so they didn't get installed. But check-plist objects to that.

mat added inline comments.Jul 30 2018, 10:28 AM
Makefile
89–90 ↗(On Diff #45744)

Mmm, if the option does nothing then do not add it.

And instead of using an OPTIONS_SINGLE, use OPTIONS_RADIO.

https://www.freebsd.org/doc/en/books/porters-handbook/makefile-options.html

154–157 ↗(On Diff #45744)

This does not really say why you cannot use COPYTREE_SHARE.

cd ${BUILD_WRKDOC} && ${COPYTREE_SHARE} . ${STAGEDIR}${DOCSDIR} '-not -name "*.1"'
Makefile
89–90 ↗(On Diff #45744)

From a user perspective it seemed much clearer to have an explicit option for it (which could have a description). Without it, it's not at all clear that no editing is even an available option.

154–157 ↗(On Diff #45744)

I just tried that, it doesn't work as given (it copies lua.1 and luac.1 anyway). Looks like COPYTREE_SHARE doesn't actually work with wildcards in the third arg, due to issues of shell quoting.

"-not -name lua.1 -not -name luac.1" works, but now it's starting to look like not so much of an improvement over the previous form.

mat added inline comments.Jul 30 2018, 1:09 PM
Makefile
89–90 ↗(On Diff #45744)

The reason we have SINGLE (1) and RADIO (0,1) is to handle those case. You can add a few words to ​EDITGRP_DESC to say that having none is ok, or add a pkg-help file to be more verbose.

But do not add a non functionnal variable only to please portlint.

154–157 ↗(On Diff #45744)

Mmmm, wildcards are supported, maybe you need to reverse the " and ' order, like "-not -name '*.1'". Or write it as '-not -name \*.1'

Makefile
89–90 ↗(On Diff #45744)

I'm OK with taking the variable out and just ignoring the warning, but changing the UI appearance just based on the fact that we don't actually need any special option processing to implement EDITNONE seems to me to be putting the cart before the horse.

154–157 ↗(On Diff #45744)

None of those work.

Makefile
154–157 ↗(On Diff #45744)

Apparently the magic incantation is to escape with \ some part of the filename pattern other than the wildcard characters; this works because of an odd interaction between shell and find(1) related to the fact that find(1) does an extra layer of unescaping for itself, while the escape character prevents the pattern from matching any files in the shell (assuming there are no files with \ in the names).

i.e. '-not -name *\.1' works

this is ... not at all well documented, while there is an example in bsd.port.mk it completely fails to point out the important aspect, and the ports handbook doesn't mention it at all. It also seems rather fragile to me.

Include mat's latest requested changes.

This revision now requires review to proceed.Aug 8 2018, 8:58 PM

So, once again, are we done here? I have nothing more to add.

mat added inline comments.Aug 13 2018, 11:23 AM
Makefile
154–157 ↗(On Diff #45744)

Damn, I knew there was something strange happening. One day I'll try to figure out what really happens and why, and document it properly.

So, once again, are we done here? I have nothing more to add.

I've asked @dbn to commit the port if there are no other comments. Perhaps a "close for comment" date of August 15?

dbn added a comment.Aug 16 2018, 6:25 PM

Apologies, I've been sick. I've kicked of an exp-run: http://dbn.westeurope.cloudapp.azure.com/lua.html

This covers the ports lang/lua53 converters/osm2pgsql databases/luadbi games/irrlamb graphics/darktable math/gnuplot net/haproxy-devel net/haproxy net/nuster www/osrm-backend.

dbn added a comment.Aug 17 2018, 6:26 AM

We have two failures and three skipped ports. The failures are do to unfetchable files from upstream. This does not affect lua, but of the dependents. I consider this a success and will commit this patch sometime during the day.

mat added a comment.Aug 17 2018, 9:13 AM

So, once again, are we done here? I have nothing more to add.

I've asked @dbn to commit the port if there are no other comments. Perhaps a "close for comment" date of August 15?

This tool, here, Phabricator, is a code review tool, not a patch queue for things to be committed, which is what Bugzilla is for.
If you are not a committer, or do not plan to commit it yourself, it is best to always open a PR on our bugzilla in parallel so that the change actually goes in our pipeline.

This revision was not accepted when it landed; it landed in state Needs Review.Aug 18 2018, 7:41 AM
Closed by commit rP477483: lang/lua53: update to 5.3.5 (final release for 5.3) (authored by dbn, committed by ). · Explain Why
This revision was automatically updated to reflect the committed changes.
dbn added a comment.Aug 18 2018, 8:17 AM
In D13690#356530, @mat wrote:

This tool, here, Phabricator, is a code review tool, not a patch queue for things to be committed, which is what Bugzilla is for.
If you are not a committer, or do not plan to commit it yourself, it is best to always open a PR on our bugzilla in parallel so that the change actually goes in our pipeline.

Just to elaborate on this a bit: Bugzilla is a good way to hold people accountable. If I neglect to commit this patch then it is easy to point out my timeout (and to get it assigned to someone else).

However, it is committed. Thank you for all the hard work :-)