Wrap manifest dependencies in functions to bypass luajit "main function has more than 65536 constants" error

[brentos]

This change wraps each dependency table in a function that returns the table to avoid the luajit limit of 65536 constants in a function.

Here is an example manifest after the change:

commands = {}
modules = {}
repository = {
   ['kong-lua-resty-kafka'] = (function () return {
      ['0.21-0'] = {
         {
            arch = "rockspec"
         }
      }
   } end)()
}

Fixes: https://github.com/luarocks/luarocks/issues/1797

[brentos]

@hishamhm or @leafo let me know if this is on the right track. A couple tests fail due to this change, I'll update those and add them to this PR.

[hishamhm]

Thanks!

@leafo will need to properly review this, but from a parallel conversation I had with him, I think ideally we'd like for the manifest modifications to apply only to the 5.1 manifest, so that the other Lua versions don't get the overhead.

[hishamhm]

@brentos I see that with the approach from this PR, the large top-level tables remain existing... I'm wondering how much the manifest can still grow with this approach.

Could you try producing a synthetic version of the manifest table with this proposed shape and try to load() it with LuaJIT, just to see how large it could grow until it would hit the problem again? That would be a useful data point to know.

[hishamhm]

@chucklean that's the intent, to make old LuaRocks versions resume working by serving an alternative shape of the manifest file which does not trigger the LuaJIT bug.

[chucklean]

@chucklean that's the intent, to make old LuaRocks versions resume working by serving an alternative shape of the manifest file which does not trigger the LuaJIT bug.

Sorry, I made that comment here, but had meant to make it in the issues thread, so moved it.

[hishamhm]

If I'm understanding the LuaJIT bug correctly and that message thread from 15 years ago, a manifest file that could grow indefinitely would have a shape somewhat like this:

commands = {}
modules = {}
repository = {}
-- collect the first n entries
(function()
   repository['foo'] ={
      ['0.21-0'] = {
         {
            arch = "rockspec"
         }
      }
   }
   repository['bar'] ={
      ['0.21-0'] = {
         {
            arch = "rockspec"
         }
      }
   }
   -- up to some n that doesn't trigger the bug?
end)()
-- collect the next n entries
(function()
   repository['bla'] ={
      ['0.21-0'] = {
         {
            arch = "rockspec"
         }
      }
   }
   repository['boo'] ={
      ['0.21-0'] = {
         {
            arch = "rockspec"
         }
      }
   }
   -- rinse and repeat
end)()
-- and so on

...either that or a (function() wrapper at the top-level? I don't know if the 65535 limit applies only to main or to the inner functions as well. Has anyone tested LuaJIT's limits?

[hishamhm]

@brentos my only question with your PR is whether the table keys in the main function count as "constants", and whether this would cause the manifest to break again once the repo grows beyond 65535 distinct package names. But there are currently around 4k package names for Lua 5.1, so it's not like this is going to break very soon. And by then we might have all moved to JSON manifests, hopefully.

@leafo I defer to you on merging this one. How does it look?

[brentos]

@hishamhm it seems to work up to at least 30,000 table keys in the way I'm generating the table. 40,000 fails in my local test. I'm just generating the file using a jinja2 template:

commands = {}
modules = {}
repository = {
{% for number in range(0, size | int) %}
   ['dep-{{ number }}'] = (function () return {
      ['0.21-0'] = {
         {
            arch = "rockspec"
         }
      }
   } end)(),
{% endfor %}
}

and using the jinja-cli to create the file: jinja -D size 10 -o manifest template.j2

[brentos]

The precise limit I hit when I use the jinja template above is 32,766 dependencies, which is almost exactly half of 65,536

[jmaher31]

@hishamhm and @leafo - we have a number of our customers waiting on this fix. Would sincerely appreciate an update on time to merge.

[mcunhap]

@hishamhm @leafo any update on this fix?

[maranza]

Hi @hishamhm @leafo is there any feedback on this fix? or a work around that was provided?

Thanks.

[mrcjkb]

question: could this potentially break the JSON manifest? Are there tests in place to ensure it doesn't?

[jgonggrijp]

@hishamhm could you ping @leafo over a side channel (such as email)? It appears he does not get notifications from this ticket.