Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No more 0 and 0 or 1 #356

Closed
wants to merge 1 commit into from
Closed

No more 0 and 0 or 1 #356

wants to merge 1 commit into from

Conversation

jlaurens
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor Author

jlaurens commented Jun 21, 2024

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
Copy link
Member

davidcarlisle commented Jun 21, 2024

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
Copy link
Contributor Author

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

l3build-setup.lua Outdated Show resolved Hide resolved
l3build-setup.lua Outdated Show resolved Hide resolved
l3build-check.lua Outdated Show resolved Hide resolved
l3build-setup.lua Outdated Show resolved Hide resolved
@davidcarlisle
Copy link
Member

@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 jlaurens force-pushed the execute51 branch 3 times, most recently from eaa51b8 to 3ffff74 Compare June 27, 2024 20:42
@jlaurens jlaurens requested a review from zauguin June 27, 2024 20:43
@jlaurens
Copy link
Contributor Author

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 jlaurens force-pushed the execute51 branch 3 times, most recently from 407cb21 to 9b13718 Compare July 4, 2024 10:48
@jlaurens jlaurens changed the title execute51 No more 0 and 0 or 1 Jul 4, 2024
@jlaurens
Copy link
Contributor Author

jlaurens commented Jul 4, 2024

@josephwright
Changes reduced to the strict minimum

@jlaurens jlaurens closed this Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants