enable relative path loading

[sebshader]

closes #10

[sebshader]

if 2 different paths resolve to the same file it will load the file twice, but it seems like pd itself does this w/ c externals.. maybe there's some lookup by absolute path that could prevent that..

[agraef]

That was a lot easier than I imagined. :) Let me give this a whirl.

[agraef]

This works, but ... it breaks the dofile and pdx live coding facilities described in https://agraef.github.io/pd-lua/tutorial/pd-lua-intro.html#dofile ff. The luatab.pd_lua file in the pdlua/tutorial/examples/luatab.pd example just never seems to be reloaded any more.

(Also check https://github.com/agraef/pd-lua/blob/master/tutorial/16-remote-control2.gif for an animated version from the tutorial.)

So your changes seem to break the dofile mechanism described there. You might not care about this, but I do. ;-) We're tantalizing close, though, so it would be a shame if we couldn't fix this. Any ideas?

[sebshader]

ah well clearly this is the issue: "NOTE: This works because the pd.Class:new():register("foo") call of the object only registers a new class if that object class doesn't exist yet; otherwise it just returns the existing class."

I changed it to register a new class every time.. this was to mimic pd's way of loading but it might be as easy as just returning existing classes again

[agraef]

Well, the filename in pdlua.c:pdlua_dofile is the right one, and the file is located correctly in pdlua/pdlua/tutorial/examples. The subsequent lua_load call works as well, otherwise it would print an error message.

I think I see what's going wrong there. Your new code in pd.lua:pd.Class:register(name) just always registers a new class now, no matter what. But it would need to check for an existing class in the case that they're exactly the same file. Can you rework it that way?

[agraef]

Yep, exactly. Sorry, our comments crossed.

[agraef]

Anyway, it shouldn't be too hard to fix this? Maybe if you first check that the class exists, then check the _scriptname member to see whether they are in fact the same?

[agraef]

And while you're at it, you should be able to remove the code for pd._check at pd.lua:47 now that it's not used in pd.Class:register(name) anymore.

[sebshader]

I think I got it. not quite sure about what's going on because I didn't look at any of what dofile is doing but it seems to work..
I don't think we need to look at _scriptname?.. (especially since 2 pd-lua classes of the same name in different directories couldn't load properly then I think)

[agraef]

You're right, that should do. Sorry, gotta run since I have to teach courses, but I'll give it another whirl later today!

[sebshader]

hmm some kinks still have to be worked out.. the 'dofile' thing is altering all of the scripts with a certain classname referenced by any path, not just the ones 'reset' is sent to.
there is another little bug I found as well.

[agraef]

Well, I just ran a quick little test and dofile is working again, but it seems that the dreaded maximum object loading depth 1000 reached rears its ugly head again as well. :(

[agraef]

Yeah, this is a bit more involved after all, the internals are a bit complicated and interwoven... Well, I'll let you sort it out while I'm away teaching. ;-)

[sebshader]

@agraef between storing the object name in an object's member like _loadname, and storing it in an external array which do you think is better?
On the one hand, storing the load name in the object could be useful to users/authors, on the other it would break any object already using _loadname and be a reserved member going forward.. (like _object)

[sebshader]

Ok I think I may have gotten it sorted but now I have to figure out how to deal w/ paths.
for instance I can't use _scriptname because dofile loads relative to the canvas (which doesn't seem great anyways if what is wanted is to load something related to the class file)
I think I might just add a 'classfile' function that searches paths relative to the class extern dir. Then the self-loading classes could use classfile instead.
or maybe I just need to update the requirepaths..

[agraef]

Sounds good. I didn't see any new commits though? About the path issue with dofile (if I understand your last comment correctly), isn't that something that can be solved by just setting up pd._loadpath in pdlua_dofile just like in the loader hook?

Anyway, I had already written a reply to your previous comment, so FWIW here goes, maybe some parts of that are still useful:

@agraef between storing the object name in an object's member like _loadname, and storing it in an external array which do you think is better?

That's a good question. Technically it belongs to the class, but it needs to be somewhere where an object can easily access it on the Lua side, so I'd put it as a member into the object itself.

And no matter what name we choose for that member, there's always the risk that some existing pdlua code might break. Maybe call it __loadname stropped with two leading underscores, and if someone happens to use that, well, tough luck. ;-)

Your key idea really was to set up that global pd._loadname variable in the loader hook so that this information is readily available to the register() method in Lua land. That might be obvious in hindsight, but ingenious ideas often are. :) I wish I had thought about this myself.

We just need to add that to pdlua_dofile as well, then everything should fall into place, hopefully. And we could just always set pd._loadname (not just when a pathname is explicitly used), I think?

[agraef]

About the path issue with dofile (if I understand your last comment correctly), isn't that something that can be solved by just setting up pd._loadpath in pdlua_dofile just like in the loader hook?

Sorry, that wasn't helpful, please ignore. dofile gets invoked with the actual script name, so setting _loadname doesn't make any sense there.

Or maybe it does if you've got that _loadname member in the object now.

Guess I'll just wait and see what you come up with. :)

[sebshader]

ok I'll push what I have so far. I'm still having some trouble getting pdluax to load though.
one peculiarity is that if a basename object is loaded after a pathname object, it will be the class of pathname. Really the pd loader makes this tricky bc it registers the 'new' function for both when the pathname class is loaded. I guess it might be possible to call class_new manually from some constructor if a pathname object is already using the new function (and basename hasn't been loaded) but that's getting pretty complex..
but at least for now pathname objects aren't interfering with basename objects that are already loaded.

[agraef]

Really the pd loader makes this tricky bc it registers the 'new' function for both when the pathname class is loaded.

Are you sure that this is in the Pd loader itself? Or is that defect only on the pdlua side?

I'm still having some trouble getting pdluax to load though.

For me pdluax just doesn't work at all in the latest iteration any more. Any idea how to get it working again? Otherwise it's a showstopper, I'm afraid. We can't just break pdluax or remove it entirely, that just won't fly with existing pdlua users.

[sebshader]

ohh I didn't catch that dofile returns multiple values..

[agraef]

Ok, looking great! I'll give it a few more hours so that we can both test the latest iteration more thoroughly with our own pdlua examples, and if nothing else turns up then I can merge it later tonight (CET).

[sebshader]

huh menu_open works fine for pdluax for me on linux

[agraef]

Hmm, that's weird. Maybe some recent change in master that's not in your branch, but there's nothing related there. So it might be a platform-specific issue. I'm on Mac right now, but I can try on Linux later tonight.

In the meantime from your latest branch I'm getting this compile error:

cc -DPD -I "/Applications/Pd-0.53-1.app/Contents/Resources/src" -DUNIX -DMACOSX -I /sw/include   -DMAKE_LIB -Ilua -DLUA_USE_MACOSX -Wall -Wextra -Wshadow -Winline -Wstrict-aliasing -O3 -ffast-math -funroll-loops -fomit-frame-pointer -arch arm64 -mmacosx-version-min=10.6 -o pdlua.o -c pdlua.c
pdlua.c:502:57: error: too few arguments to function call, expected 5, have 4
            if (lua_load(__L, pdlua_reader, &reader, buf))
                ~~~~~~~~                                ^
lua/lua.h:289:16: note: 'lua_load' declared here
LUA_API int   (lua_load) (lua_State *L, lua_Reader reader, void *dt,
               ^
1 error generated.
make: *** [pdlua.o] Error 1

That looks like an editing blunder in rev. 3b94c67. Here's the culprit:

+#if LUA_ION_NUM        < 502
+            if (lua_load(__L, pdlua_reader, &reader, buf))
+#else // 5.2 style
+            if (lua_load(L, pdlua_reader, &reader, filename, NULL))
+#endif // LUA_VERSION_NUM      < 502

[agraef]

There's still an L where there should be an __L. I'll just do that myself and merge directly from my branch, so that I can also squash to make the history look a bit tidier. No need to keep every little fixup commit.

[agraef]

Ok, there you go: master...relative_path_loader

As you can see I cleaned this up a bit by squashing all the minor corrections and splitting out the typo fix to pdluax-help.pd into its own commit. The contents is the same as your branch + the compile fixes for Lua 5.2+ I added:

diff --git a/pdlua.c b/pdlua.c
index a1559c9..21800f7 100644
--- a/pdlua.c
+++ b/pdlua.c
@@ -501,7 +501,7 @@ static t_pdlua *pdlua_new
 #if LUA_VERSION_NUM    < 502
             if (lua_load(__L, pdlua_reader, &reader, buf))
 #else // 5.2 style
-            if (lua_load(L, pdlua_reader, &reader, filename, NULL))
+            if (lua_load(__L, pdlua_reader, &reader, buf, NULL))
 #endif // LUA_VERSION_NUM      < 502
             {
                 close(fd);

If that looks good to you, I can just pull it over into master, and we consider this PR done. If it doesn't then feel free to edit as needed and force-push back into this branch so that I can merge. Thanks.

[sebshader]

seems right to me

[agraef]

Great! I already gave this a fairly thorough workout using all the examples from the tutorial. Everything seems to work fine now, but I will also check some of the other examples and other pdlua stuff I've written, just to make sure that we don't break any existing code.

I also found the issue with pdluax menu_open not working with relative paths, it's this line here (pd.lua:932):

    self._scriptname = pathname .. '/' .. atoms[1] .. ".pd_luax" -- mrpeach 20120201

That used to work when we had no support for relative pathnames, but will fail if a part of the pathname also appears as a prefix in atoms[1], resulting in duplication. We just need to use basename(atoms[1]) instead then everything will be fine. I will add that in a moment.

[agraef]

Fixed in rev. b5f53bc.

[agraef]

Merged with rev. f5d0bb9. Thanks!

[sebshader]

@agraef great!
I know that you already merged it, but regarding doclassfile vs dofile: I think there are situations where an external written in lua would want to reference its load location with dofile rather than its distributed location with doclassfile (like when its loading files other than itself, scripts that might be distributed w/ a patch the external is used in, or in abstraction libraries etc.)

for instance I have a library lscore and though I'm not sure I'll transition to dofile (just because I already wrote my own extension to do so in parallel with pdlua) I want users to be able to load scores written in lua that can be distributed in patches that the [lscore] object is loaded in. I could maybe do something like that with dofile. then, the composition/score could be distributed with the patch that implements the 'instruments'

so I still think it makes sense to have them separate from the user's perspective (not to mention that now dofile isn't backwards compatible afaict)

[agraef]

Sure. This is very complicated and I'm still struggling to understand the differences between dofile and doclassfile, to be honest. But thanks for pointing out your use case, it makes perfect sense to me.

So your suggestion is to revert rev. f2a889c?

[sebshader]

@agraef yes
the difference is just where the search path looks.
let's say the class.pd_lua file lives at /lib directory
you have a patch that uses [class] in a different directory /patches.
dofile will look for lua scripts in the /patches directory because that's where the canvas is
doclassfile will load scripts from /lib directory because that is where the file resides.
both have their uses imo

[agraef]

So let me try to get this straight, I really need to understand what's going on there, so that I can describe it correctly in the tutorial.

So, before we had any relative paths, the path of the class (i.e., its externdir) was just always the same as the path of the object (which is the path of the canvas with the object), so dofile would just work for both use cases.

But now we have to make sure that the class file is always loaded from its original externdir (which isn't necessarily the same as the canvas dir because of relative paths), whereas we still expect other Lua files to be loaded relative to the canvas dir (with one example being the pdlua and pdluax objects).

Did I get this right? At least it makes sense to me. :)

[agraef]

Ok, thanks for elaborating. So I think I understand this now. Joy. :)

[sebshader]

yes. tbh I'm not completely sure on the difference between the paths pd looks in for canvas_path, and using externdir but using dofile didn't work right for reloading the patch file.
actually maybe now that absolute paths are used we could just load by absolute path, but then that would be up to the user using self._loadpath. by using doclassfile the user can continue loading paths like they were before
(because it used to be that 'name' referred to the file path as well because it wasn't relative, but now it doesn't reflect the relative path to the canvas)

[agraef]

Ok. I still don't like the name doclassfile, though. It's an awful lot to type, but more importantly it suggests that you only use it for reloading class files, when you can actually use it to load any Lua code relative to the external (rather than the object canvas). Trying to think about a better name.

[agraef]

doxfile maybe, short for "dofile from extern dir"? Yeah, it's ugly. ;-) Do you have a better idea?

[sebshader]

hmm do_externdir? I struggled w/ that too

[agraef]

Nah, I think they should sound about the same, just different enough to convey the meaning. I think that doxfile isn't so bad after all, I'll go with that for now. If you still have a better idea, just let me know.

[sebshader]

dofilex? dofileext? just tossing stuff out idk

[agraef]

dofilex it is, I like it. :)

BTW, I can confirm that dofile also works if the object is in the same path as the external. That's good because it means that existing code and patches written without relative paths will continue to work. But of course pdx.lua uses dofilex now.

Ok, now I just need to edit the tutorial accordingly.

[agraef]

BTW, can you confirm that the present version still works with Lua 5.1? I mean, not just with some simple test examples, but for substantial applications like that library of yours that you mentioned earlier. Then I should add that information to the README. Currently, Lua 5.1 is listed as "completely unsupported" there.

[agraef]

All done and dusted, retagged 0.11.2, I can hopefully add some release notes and binaries over the weekend.

[sebshader]

well lscore has still been in WIP state for years but the 2 tests I had for it passed fine w/ luajit 5.1
I suppose 'completely unsupported' doesn't necessarily mean 'non-working'

[agraef]

I suppose 'completely unsupported' doesn't necessarily mean 'non-working'

Yeah, I cautiously reworded that passage in the README for the release. :) No need to scare people away from giving it a try at least, and your project proves that it works to some extent at least.

Unfortunately, we don't have a test suite yet, which would make these checks much easier. I really need to look at how Jonathan implemented this for Purr Data.

But first I need to look at GH workflows to automate the builds. My last foray into CI on GH was in the bad old Travis days, I think that things have been improved a lot since then. I can see that Tim uses it to build plugdata, but I'm not sure how much of that runs on his own servers, or how much actual money it costs to use GH's build farm.