premake / premake-core

Premake
https://premake.github.io/
BSD 3-Clause "New" or "Revised" License
3.22k stars 620 forks source link

Modules '_preload.lua' only executed for embedded modules #1235

Open tritao opened 5 years ago

tritao commented 5 years ago

This has caught me by surprise, but it looks like modules _preload.lua is only called for built-in embedded modules in _premake_main.lua:preloadModules.

It's a little bit weird, I would expect _preload.lua to be called when requiring any module, embedded or not. In fact, I can see that some modules have some hacks inside to workaround this issue.

Is this by design?

ttvd commented 5 years ago

I am curious about this as well; I have to always edit the _modules.lua list in order to compile my own modules into a binary premake (after embedding).

starkos commented 5 years ago

How would that work? Would you have to scan every folder along Premake's search path looking for _preload.lua files on startup? We do it for core modules because a) we already know where those files live, and b) it helps reduce startup time.

A workaround is to reference the scripts you want to preload from your system script

dofile('mymodule1/_preload.lua')
dofile('mymodule2/_preload.lua')

…so they will load each time Premake starts, without incurring the cost of scanning the disk.

samsinsane commented 5 years ago

@starkos I'm not sure what @tritao was imagining, but I would expect require to call _preload.lua first. Also, maybe we need a better way to include modules without loading them in their entirety? Given that a number of modules extend vstudio, it causes all of vstudio to be loaded even when not using a VS action.

WorldofBay commented 5 years ago

require is just a dofile with memory (built-in lua)

however, wrapping it in an internal function could work like

function load_module(name) {
  assert(type(name) == "string", "module name is required!")
  local preload = io.open(name.."/_preload.lua")
  if preload then
    io.close(preload)
    require(name.."/_preload")
  end
  require(name)
}

then the wiki needs to tell people to use that function instead of require

but for what do modules need this when they can just put their _preload code in their main file? is there any reason to not put the code in the main file?

samsinsane commented 5 years ago

is there any reason to not put the code in the main file?

It creates an unnecessary inconsistency between the two types of modules, internal and external. For example:

My suggestion is that we treat internal and external modules the same, the _preload.lua function isn't run until after the baking process anyway, so the entire premake5.lua file has been executed. I think we should have a registermodule function which would work similar to the internals of the for loop here: https://github.com/premake/premake-core/blob/ba3b2eb50b70eb0d85c55512b9cd3814a8237a71/src/_premake_main.lua#L115-L129

This would then become the recommended way to consume modules in your scripts, but require would continue to be used inside of modules to extend other modules.

WorldofBay commented 5 years ago

on the modules extending vstudio:

depends on how much they extend, vstudio module should not have to know what the callers want but maybe there is a good way to modularize it further?

having modules and having those modules depend on each other is good. having modules and having those modules reuse stuff from each other is rather bad. either that stuff was wrong to land in a module or the reusing module should strongly reconsider if it actually needs a whole module for a single function ...

thinking out of the box, there may be a reason for kind of a vstudio-core module that contains the base stuff other modules build on.

starkos commented 5 years ago

If you can come up with a better approach, I'm all for it. I felt having a preload script for the internal modules was a useful optimization. If it is causing confusion, I have no issue with them being removed, and all logic moved into the main module.

If you want to generalize this to all modules, the easiest way may be to put the burden on the caller:

require('emscripten/loader')  -- this `requires('emscripten')`as needed
samsinsane commented 5 years ago

@WorldofBay The problem I raised wasn't the amount that was pulled in via require but that _preload.lua functions are ultimately ignored. I provided an example of how this occurs and I think this has confused you.

@starkos As I suggested, the registermodule would be simple and wouldn't really change anything drastic? I think the optimization is good, but as I've said, it's bypassed by external modules and given how easy this is to resolve I think it's quite an unnecessary bypass to occur.

starkos commented 5 years ago

Sorry, that slipped through: yes, registermodule sounds like a good approach.

samsinsane commented 5 years ago

@starkos No worries, if I ever get a chance I might look into this, it seems so simple but just requires some time to do it.

starkos commented 3 years ago

Just a heads up that register('module_name') is now supported in v6.x (not much else is yet though). It looks for and runs a script called _register.lua in the module's folder.