(release 30) Linux generic installer fixes by vadi2 · Pull Request #414 · Mudlet/Mudlet

vadi2 · 2017-03-09T07:29:08Z

Need to fix the Linux generic installer - as the current one does not work, at all, for anybody. Switching over to using AppImage instead of BitRock as I've validated AppImage with two random Ubuntu users and it at least seems to work better. Plus, it seems to be a better thing for the Linux ecosystem in general.

These fixes cherrypick 7922a3a from the development branch, improve that function to return multiple paths, and in general, make Mudlet also search things relative to its binary using an absolute path. This is because AppImage does not set the current working directory for us unlike what Mudlet has been used to.

Merging this will also require merging which is a fix to LuaGlobal to also load stuff relative to the executable using a relative path.

Mostly getting Mudlet to look for files using an absolute path, because AppImage doesn't run set the current working directory for us. Also updated LuaGlobal C++ searchpath to include absolute path. This is necessary for AppImage to work on Linux, which does not set the current working directory to be the mudlet executable.

Add compile-time fallback to LuaGlobal.lua loading logic to attempt to load from LUA_DEFAULT_PATH, defined in src/src.pro using the value from LUA_DEFAULT_DIR. For platforms that don't currently define LUA_DEFAULT_DIR in src/src.pro (e.g. win32, macx), the loading logic will behave as though LUA_DEFAULT_DIR were defined as "/", the root directory. This is accomplished by compile time concatenation of LUA_DEFAULT_PATH and the string literal "/". If necessary, LUA_DEFAULT_DIR can later be defined for additional platforms (win32, macx, etc) in src/src.pro to provide a different compile-time fallback for that platform. (cherry picked from commit ee48788)

…locations Added capability to return a LuaGlobal search path relative to the Mudlet binary - needed for AppImage on Linux which does not set the current working directory to the binary for us. Renamed getMudletLuaDefaultPath() to getMudletLuaDefaultPaths() as it hasn't been out in a release yet and we need to return a list of values.

This is the standard - search paths closest first and then those further away. Makes it easier to modify settings if need be.

keneanung · 2017-03-09T22:03:26Z

src/TLuaInterpreter.cpp

+    lua_newtable( L );
+    lua_createtable(L,2,0);
+    // add filepath relative to the binary itself (one usecase is AppImage on Linux)
+    QString nativePath = QDir::toNativeSeparators( QCoreApplication::applicationDirPath() + "/mudlet-lua/lua/" );


I have no idea about C++, but this is not the correct path to look at for macOS. The code a few lines below works for locating the LuaGlobal.lua, though:

#if defined(Q_OS_MAC) QString path = QCoreApplication::applicationDirPath() + "/../Resources/mudlet-lua/lua/LuaGlobal.lua"; #else

Could you add this in? If so, we might be able to get rid of the run_mudlet helper.

The macOS code is already in there for a long while, this is a Linux-specific fix.

The macOS code only worked, because of the run_mudlet workaround, which was necessary, because, without that, the working directory is /. I only understood why, because you practically put the solution under my nose. We should be able to get rid of the workaround for good if we add the conditional compile in.

Otherwise, I'll make the changes and send in an extra PR if that's your preferred way.

Well "Allow edits from maintainers." is ticked, I think it'll easiest to communicate if you add the changes you'd like to see. Not getting you 100% here.

Oh, it returns a table! I was somehow reading it as returning a single string (mostly in connection with your changes to LuaGlobal.cs

vadi2 · 2017-03-10T07:40:50Z

I think I understand it now, macOS had the same issue as the AppImage in regards to current working directory not being set, and I fixed that at the time with run_mudlet.sh. Here I've adapted Mudlet to cope with it instead. You should be able to just add the macOS path to the list of paths the C++ function returns for it to work.

…

On Thu, 9 Mar 2017 11:11 pm keneanung, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/TLuaInterpreter.cpp <#414 (comment)>: > @@ -5182,6 +5182,23 @@ int TLuaInterpreter::getMudletHomeDir( lua_State * L ) return 1; } +// returns search paths for LuaGlobal itself to look at when loading other modules +// follows the principle of closest paths to the binary first, furthest away lasy +int TLuaInterpreter::getMudletLuaDefaultPaths( lua_State * L ) +{ + lua_newtable( L ); + lua_createtable(L,2,0); + // add filepath relative to the binary itself (one usecase is AppImage on Linux) + QString nativePath = QDir::toNativeSeparators( QCoreApplication::applicationDirPath() + "/mudlet-lua/lua/" ); The macOS code only worked, because of the run_mudlet workaround, which was necessary, because, without that, the working directory is /. I only understood why, because you practically put the solution under my nose. We should be able to get rid of the workaround for good if we add the conditional compile in. Otherwise, I'll make the changes and send in an extra PR if that's your preferred way. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#414 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGxjLfB2rCtMROgNyC8GaWmYWS-cX4xks5rkHjxgaJpZM4MXvCo> .

keneanung · 2017-03-10T09:34:26Z

src/mudlet-lua/lua/LuaGlobal.lua

 local prefixes = {"../src/mudlet-lua/lua/", "../Resources/mudlet-lua/lua/",
-    "mudlet.app/Contents/Resources/mudlet-lua/lua/", "mudlet-lua/lua"}
+    "mudlet.app/Contents/Resources/mudlet-lua/lua/", "mudlet-lua/lua",
+    getMudletLuaDefaultPath()}


Typo: You defined the function as getDefaultPaths (missing an "s" here)

The function returns a table right now, should probably add an unpack here.

@keneanung

getMudletLuaDefaultPaths != getMudletLuaDefaultPath
AND
getMudletLuaDefaultPath != getDefaultPaths 😜 - but well spotted!

Does not the following code (lines 133-150 old; lines 134-151 new) handle stepping through the table?

Just to explain... The function returns a table, so the table is now {"path1", "path2", {"path from C", "another path from C"}}. The loop below iterates over the outer table only, you want both tables merged.

But the actual LuaGlobal is in#415

Could you approve this PR? #415 is where the table iteration is fixed.

With this change, we should be able to get rid of the `run_mudlet` workaround for the macOS Mudlet.app package.

vadi2 · 2017-03-10T11:40:57Z

Egh github email reply sync is slow. Anyway there's no typo, I just forgot to link the or in description, see #415 that follows.

keneanung · 2017-03-10T11:50:31Z

Maybe don't change LuaGlobal here, if it's not correct anyways?

vadi2 · 2017-03-10T11:52:22Z

It came as part of the commit I cherry picked from development. Changing this to be a submodule after 3.0 will prevent this kind of confusion, so not long to go!

…

On Fri, 10 Mar 2017 12:50 pm keneanung, ***@***.***> wrote: Maybe don't change LuaGlobal here, if it's not correct anyways? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#414 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGxjBBIBcqPePyvrxAU6By1CJVJtpJOks5rkTkIgaJpZM4MXvCo> .

To avoid confusion about search paths, the macOS Lua default path is only added when compiling for macOS.

keneanung · 2017-03-10T12:22:45Z

But now you need to make sure the branches are merged in the correct order.

There, added the #ifs, though the code became a bit less readable this way...

SlySven · 2017-03-10T17:55:27Z

src/TLuaInterpreter.cpp

 }

+// returns search paths for LuaGlobal itself to look at when loading other modules
+// follows the principle of closest paths to the binary first, furthest away lasy


lasy ==> later/lastly?

SlySven · 2017-03-10T17:57:00Z

src/TLuaInterpreter.cpp

+#if defined(Q_OS_MAC)
+    lua_createtable(L,3,0);
+#else
+    lua_createtable(L,2,0);


I'm not a Lua genius, so is a comment about why there is a difference here for Macs appropriate?

Explained by the add macOS lua path relative to the binary itself, which is part of the Mudlet.app package comment below.

SlySven · 2017-03-10T18:09:43Z

src/TLuaInterpreter.cpp


+#ifdef Q_OS_LINUX
    // if using LuaJIT, adjust the cpath to look in /usr/lib as well - it doesn't by default
    luaL_dostring (pGlobalLua, "if jit then package.cpath = package.cpath .. ';/usr/lib/lua/5.1/?.so;/usr/lib/x86_64-linux-gnu/lua/5.1/?.so' end");


I think we have dropped support for LuaJIT in the development branch - this line was removed some time back from that branch IIRC...!

We'll clean it up in the release_30 merge into development.

SlySven · 2017-03-10T18:12:07Z

src/TLuaInterpreter.cpp

    {
        // For the installer we do not go down a level to search for this. So
        // we check again for the user case of a windows install.
+#if QT_VERSION >= 0x050200


We no longer support Qt 4.x (except in a port I keep on my Repo... 😏)

SlySven · 2017-03-10T18:16:40Z

src/TLuaInterpreter.cpp

+#else
        path = "mudlet-lua/lua/LuaGlobal.lua";
+#endif
        error = luaL_dofile( pGlobalLua, path.toLatin1().data() );


We might want to substitute path.toUtf8().constData() for path.toLatin1().data() - (use utf8() for those Windows machines who's user name and thus path contains non-ASCII characters; and constData() because we do not need to/want to/can change the supplied string which is the raw characters of path IIRC).

SlySven · 2017-03-10T18:18:59Z

src/TLuaInterpreter.cpp

    }

+    // Finally try loading from LUA_DEFAULT_PATH
+    path = LUA_DEFAULT_PATH "/LuaGlobal.lua";


ℹ️ This is string catenation isn't it?

SlySven · 2017-03-10T18:20:02Z

src/TLuaInterpreter.cpp


+    // Finally try loading from LUA_DEFAULT_PATH
+    path = LUA_DEFAULT_PATH "/LuaGlobal.lua";
+    error = luaL_dofile( pGlobalLua, path.toLatin1().data() );


Another place for path.toUtf8().constData() to be considered...

vadi2 · 2017-03-10T18:31:08Z

See next commit for handling a table output!

…

On Fri, 10 Mar 2017 7:29 pm Stephen Lyons, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/mudlet-lua/lua/LuaGlobal.lua <#414 (comment)>: > @@ -128,7 +128,8 @@ local packages = { -- TODO: extend to support common Lua code being placed in system shared directory -- tree as ought to happen for *nix install builds. local prefixes = {"../src/mudlet-lua/lua/", "../Resources/mudlet-lua/lua/", - "mudlet.app/Contents/Resources/mudlet-lua/lua/", "mudlet-lua/lua"} + "mudlet.app/Contents/Resources/mudlet-lua/lua/", "mudlet-lua/lua", + getMudletLuaDefaultPath()} @keneanung <https://github.com/keneanung> 1. getMudletLuaDefaultPaths != getMudletLuaDefaultPath AND getMudletLuaDefaultPath != getDefaultPaths 😜 - but well spotted! 2. Does not the following code (lines 133-150 old; lines 134-151 new) handle stepping through the table? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#414 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGxjKbEs36oQoPYLVRe6VXefjagxm9Cks5rkZZ5gaJpZM4MXvCo> .

vadi2 · 2017-03-10T18:31:23Z

I mean PR

…

On Fri, 10 Mar 2017 7:30 pm Vadim Peretokin, ***@***.***> wrote: See next commit for handling a table output! On Fri, 10 Mar 2017 7:29 pm Stephen Lyons, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/mudlet-lua/lua/LuaGlobal.lua <#414 (comment)>: > @@ -128,7 +128,8 @@ local packages = { -- TODO: extend to support common Lua code being placed in system shared directory -- tree as ought to happen for *nix install builds. local prefixes = {"../src/mudlet-lua/lua/", "../Resources/mudlet-lua/lua/", - "mudlet.app/Contents/Resources/mudlet-lua/lua/", "mudlet-lua/lua"} + "mudlet.app/Contents/Resources/mudlet-lua/lua/", "mudlet-lua/lua", + getMudletLuaDefaultPath()} @keneanung <https://github.com/keneanung> 1. getMudletLuaDefaultPaths != getMudletLuaDefaultPath AND getMudletLuaDefaultPath != getDefaultPaths 😜 - but well spotted! 2. Does not the following code (lines 133-150 old; lines 134-151 new) handle stepping through the table? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#414 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGxjKbEs36oQoPYLVRe6VXefjagxm9Cks5rkZZ5gaJpZM4MXvCo> .

SlySven

Generally 👍 approval-able with that getMudletLuaDefaultPath__s__ typo fixed.

vadi2 · 2017-03-12T19:55:27Z

Hm... tried out the mac fix and it didn't help, mostly because Mudlet can't find the Lua libraries it needs to begin with. I think run_mudlet.sh is still needed to get those to work - that or modifying the Mudlet binary to search for additional rpaths on macOS.

Nonetheless, getting rid of run_mudlet.sh on macs isn't in scope on this PR - @keneanung could you approve this one?

keneanung · 2017-03-12T20:02:06Z

Let me have one last look at it, I was just booting my hackintosh

keneanung · 2017-03-12T20:59:57Z

@vadi2 found the issue that prevented it from working. The macOS path was missing a / Between the application path and the manual addition...

vadi2 · 2017-03-13T01:04:11Z

It didn't seem to make a difference for me - but I could also be testing it wrong. The right way to do it would be to remove the run_mudlet.sh script and get rid of the plist editing we do - I was just trying to run /MacOS/Mudlet directly.

7922a3a added getMudletLuaDefaultPath() that returned a single path, whereas the PR Mudlet#414 changed function to be getMudletLuaDefaultPaths() and made it return an indexed table. This fixes the wrong call that got left behind between cherrypicking and merging.

keneanung · 2017-03-13T08:20:05Z

t didn't seem to make a difference for me - but I could also be testing it wrong.

The problem is, that the way we load LuaGlobal.lua doesn't differentiate between File not Found and a compile error during dofile. And since the LuaGlobal in this PR was not working, it tried the other paths as well. We should correct that behaviour after 3.0 is released.

vadi2 · 2017-03-13T08:29:33Z

Aha, true. Will see how it is after #421 is in.

7922a3a added getMudletLuaDefaultPath() that returned a single path, whereas the PR #414 changed function to be getMudletLuaDefaultPaths() and made it return an indexed table. This fixes the wrong call that got left behind between cherrypicking and merging.

7922a3a added getMudletLuaDefaultPath() that returned a single path, whereas the PR Mudlet#414 changed function to be getMudletLuaDefaultPaths() and made it return an indexed table. This fixes the wrong call that got left behind between cherrypicking and merging.

* Changes to make AppImage install working on Linux Mostly getting Mudlet to look for files using an absolute path, because AppImage doesn't run set the current working directory for us. Also updated LuaGlobal C++ searchpath to include absolute path. This is necessary for AppImage to work on Linux, which does not set the current working directory to be the mudlet executable. * Bugfix/Enhance: Load LuaGlobal.lua from LUA_DEFAULT_PATH Add compile-time fallback to LuaGlobal.lua loading logic to attempt to load from LUA_DEFAULT_PATH, defined in src/src.pro using the value from LUA_DEFAULT_DIR. For platforms that don't currently define LUA_DEFAULT_DIR in src/src.pro (e.g. win32, macx), the loading logic will behave as though LUA_DEFAULT_DIR were defined as "/", the root directory. This is accomplished by compile time concatenation of LUA_DEFAULT_PATH and the string literal "/". If necessary, LUA_DEFAULT_DIR can later be defined for additional platforms (win32, macx, etc) in src/src.pro to provide a different compile-time fallback for that platform. (cherry picked from commit ee48788) * Updated getMudletLuaDefaultPath+s to return a table of viable search locations Added capability to return a LuaGlobal search path relative to the Mudlet binary - needed for AppImage on Linux which does not set the current working directory to the binary for us. Renamed getMudletLuaDefaultPath() to getMudletLuaDefaultPaths() as it hasn't been out in a release yet and we need to return a list of values. * Changed load order to be 'closest to binary first' This is the standard - search paths closest first and then those further away. Makes it easier to modify settings if need be. * Add macOS lua path to getMudletLuaDefaultPaths With this change, we should be able to get rid of the `run_mudlet` workaround for the macOS Mudlet.app package. * Add Mac lua default path only when compiling there To avoid confusion about search paths, the macOS Lua default path is only added when compiling for macOS.

7922a3a added getMudletLuaDefaultPath() that returned a single path, whereas the PR #414 changed function to be getMudletLuaDefaultPaths() and made it return an indexed table. This fixes the wrong call that got left behind between cherrypicking and merging.

vadi2 and others added 4 commits March 9, 2017 00:55

Changed load order to be 'closest to binary first'

ecd9211

This is the standard - search paths closest first and then those further away. Makes it easier to modify settings if need be.

vadi2 requested a review from SlySven March 9, 2017 07:29

vadi2 added the 1 - Ready label Mar 9, 2017

vadi2 mentioned this pull request Mar 9, 2017

Sync mudlet-lua to 9f9248fdb2900f695cb6df7a99bbcbd2135dab55 #415

Merged

keneanung requested changes Mar 9, 2017

View reviewed changes

keneanung requested changes Mar 10, 2017

View reviewed changes

Add macOS lua path to getMudletLuaDefaultPaths

897e7a8

With this change, we should be able to get rid of the `run_mudlet` workaround for the macOS Mudlet.app package.

Add Mac lua default path only when compiling there

307a342

To avoid confusion about search paths, the macOS Lua default path is only added when compiling for macOS.

Use correct index variable

9d3d123

SlySven reviewed Mar 10, 2017

View reviewed changes

SlySven approved these changes Mar 10, 2017

View reviewed changes

Fix typo.

955fee4

Add a missing slash in the mac default path

312f840

keneanung approved these changes Mar 12, 2017

View reviewed changes

vadi2 merged commit c116a90 into Mudlet:release_30 Mar 13, 2017

vadi2 deleted the (release_30)-generic-installer-fixes branch March 13, 2017 01:04

vadi2 mentioned this pull request Mar 13, 2017

(release_30) Undo call to getMudletLuaDefaultPath() #421

Merged

keneanung mentioned this pull request Mar 19, 2017

(release_30) Modify the package.cpath for MacOS as well #429

Merged

keneanung mentioned this pull request Mar 21, 2017

(dev) Lua search path adaptions #430

Merged

mehulmathur16 pushed a commit to mehulmathur16/Mudlet that referenced this pull request Feb 16, 2024

fix: Include correct windows header for _wfullpath (Mudlet#414)

0988b0b

Uh oh!

Conversation

vadi2 commented Mar 9, 2017

Uh oh!

keneanung Mar 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vadi2 Mar 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vadi2 commented Mar 10, 2017 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vadi2 commented Mar 10, 2017

Uh oh!

keneanung commented Mar 10, 2017

Uh oh!

vadi2 commented Mar 10, 2017 via email

Uh oh!

keneanung commented Mar 10, 2017

Uh oh!

SlySven Mar 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vadi2 commented Mar 10, 2017 via email

Uh oh!

vadi2 commented Mar 10, 2017 via email

Uh oh!

SlySven left a comment

Choose a reason for hiding this comment

Uh oh!

vadi2 commented Mar 12, 2017

Uh oh!

keneanung commented Mar 12, 2017

Uh oh!

keneanung commented Mar 12, 2017

Uh oh!

vadi2 commented Mar 13, 2017

Uh oh!

keneanung commented Mar 13, 2017

Uh oh!

vadi2 commented Mar 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

keneanung Mar 9, 2017 •

edited

Loading

vadi2 Mar 9, 2017 •

edited

Loading

SlySven Mar 10, 2017 •

edited

Loading