latex3 / l3build

A testing and building system for LaTeX
LaTeX Project Public License v1.3c
88 stars 14 forks source link

No more 0 and 0 or 1 #356

Closed jlaurens closed 2 months ago

jlaurens commented 3 months ago

l3build-core.lua defines execute51 and a local require

The old build_require function made little sense, so it has been removed.

The implementation of os.execute in texlua is not that of Lua 5.3. It is still at version 5.1. See https://tug.org/pipermail/luatex/2015-November/005536.htm.

Moreover, os.execute version 5.3 is buggy on windows in certain situations. This has been fixed in Lua 5.4. If texlua ever switches to Lua 5.4, we just have to redefine execute51 et voilà.

Instead of adding more global variables, the l3build-setup module returns a table with appropriate fields. This table is globally available with require("l3build"). Actually, this is for development only, which means that the build.lua and various config-....lua are not expected to use this.

josephwright commented 3 months ago

I'm not sure what the motivation is here: things work fine with current LuaTeX, and if there is a need for a change, we'll adjust l3build at that stage. (I believe that a change is unlikely due to the security restrictions needed for TeX Live.)

jlaurens commented 3 months ago

Note the typo, this is not "l3build-core" but "l3build-setup"...

Concerning execute51.

Official documentation on the internet states that os.execute first return value is a boolean. This is silently not the case in luatex. Deliberately doing things contrary to the official documentation must be clearly documented and exposed.

Actually l3build is partly broken because of some constructs like

return os.execute(...) and 0 or 1

that always return 0 because 1 and 0 or 1 is 0. This PR fixes these bugs. These bugs appear when l3build cannot perform its tasks properly, so quite never in practice... Moreover, this is not (yet) covered by testing.

Using a execute51 function clearly states that it is not the official os.execute that one would expect from a tool that claims to implement Lua 5.3.

On a different matter, the same problem occurs in the penlight library shipped in texlive.

Concerning build_require: This local function did not add anything interesting.

davidcarlisle commented 3 months ago

Using a execute51 function clearly states that it is not the official os.execute that one would expect from a tool that claims to implement Lua 5.3.

I think that would be misleading in this context though as l3build does not claim to run with a stock Lua at all, and it does use the os.execute provided by texlua, not (as this change would imply) an overloaded version local to l3build.

jlaurens commented 3 months ago

@davidcarlisle In the last commit, execute51 prints a message in debug mode.

davidcarlisle commented 3 months ago

@davidcarlisle In the last commit, execute51 prints a message in debug mode.

But it still introduces the misleading impression that l3build is using some local non standard os.execute it isn't using a Lua 5.1 function, it is using the standard os.execute on the only platform on which the code runs, which is texlua which is not a stock Lua 5.3 or 5.1 but some forked version of Lua with some aspects of both and some tex-specific libraries and other changes.

jlaurens commented 3 months ago

The bugs are still fixed I have removed the require business which is turned into an issue. execute51 is renamed back to execute The l3build-setup.lua is still there, but it is now named l3buildlib.lua. This file will contain machinery for custom options.

jlaurens commented 3 months ago

@josephwright Changes reduced to the strict minimum