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

hs.configdir resolution change in 0.9.79 breaks module loading for symlinked configs #2478

Closed
luckman212 opened this issue Sep 20, 2020 · 16 comments Β· Fixed by #2490
Closed

hs.configdir resolution change in 0.9.79 breaks module loading for symlinked configs #2478

luckman212 opened this issue Sep 20, 2020 · 16 comments Β· Fixed by #2490

Comments

@luckman212
Copy link
Contributor

luckman212 commented Sep 20, 2020

Thanks @asmagill, @cmsj, @latenitefilms β€” and to everyone else who contributed to this new release! πŸš€

I've been keeping my configs in sync between multiple Macs by symlinking init.lua (and other modules) into ~/.hammerspoon from a central config dir (~/Sync/Settings/Hammerpoon) using Resilio Sync.

My config failed to load when upgrading from 0.9.78. In the console, the following errors were logged:

...app/Contents/Resources/extensions/hs/_coresetup/init.lua:651: module 'keybinds' not found:
	no field package.preload['keybinds']
	no file '/Users/luke/Sync/Settings/Hammerspoon/keybinds.lua'
	no file '/Users/luke/Sync/Settings/Hammerspoon/keybinds/init.lua'
	no file '/Users/luke/Sync/Settings/Hammerspoon/Spoons/keybinds.spoon/init.lua'
        < ...snip... >
stack traceback:
	[C]: in function 'rawrequire'
	...app/Contents/Resources/extensions/hs/_coresetup/init.lua:651: in function 'require'
	(...tail calls...)
	[C]: in function 'xpcall'
	...app/Contents/Resources/extensions/hs/_coresetup/init.lua:514: in function <...app/Contents/Resources/extensions/hs/_coresetup/init.lua:494>

I eventually figured out that this was due to a change in the way hs.configdir is resolved. Previously, it seems like it would resolve to ~/.hammerspoon but now, if init.lua is itself a symlink, then hs.configdir is set to the parent directory of the absolute path of the real file. This is probably fine for 99.9% of usersβ€” but in my case, it broke my configuration.

I have the following symlinks:

~/.hammerspoon/init.lua       β†’  ~/Sync/Settings/Hammerspoon/init-XXX-YYY.lua
~/.hammerspoon/keybinds.lua   β†’  ~/Sync/Settings/Hammerspoon/keybinds-XXX-YYY.lua

(where XXX=hostname and YYY=machine serial number)

The reason I do this is so I can share chunks of configuration in init.lua between multiple Macs, but have the keybinds/module settings be specific to the machine that's loading them. I symlink a custom file for each machine but they are all named e.g. "keybinds.lua".

I "fixed" it by adding a function to my init.lua:

function require_safe(m)
  local rs_abspath = hs.fs.pathToAbsolute(os.getenv('HOME') .. '/.hammerspoon/' .. m .. '.lua')
  if not rs_abspath then return end
  local rs_basename = string.gsub(rs_abspath, "(.*/)(.*)", "%2")
  local rs_modname = rs_basename:sub(1, -5)
  return require(rs_modname)
end

-- instead of require('keybinds')
require_safe('keybinds')

This has gotten things working again for me. But I'm guessing there's a cleaner way to handle this...?

@luckman212
Copy link
Contributor Author

luckman212 commented Sep 21, 2020

Re-tested with 0.9.80, just in case. Same problem.

I found a simpler workaround though, adding these 2 lines at the top of init.lua:

hs.configdir = os.getenv('HOME') .. '/.hammerspoon'
package.path = hs.configdir .. '/?.lua;' .. hs.configdir .. '/?/init.lua;' .. hs.configdir .. '/Spoons/?.spoon/init.lua;' .. package.path

(update: added /Spoons as per @smferris' suggestion below)

I searched the source to try and find where configdir gets initialized, but I'm just not seeing it. I see it's used in setup.lua#L3. Is this a Lua builtin or something?

@smferris
Copy link

I had the same problem since I use homeshick to manage my dotfiles in a git repo, and all of my dotfiles are symlinks.

If you want Spoons to load, you'll need those in package.path as well:

hs.configdir = os.getenv('HOME') .. '/.hammerspoon'
package.path = hs.configdir .. '/?.lua;' .. hs.configdir .. '/?/init.lua;' .. hs.configdir .. '/Spoons/?.spoon/init.lua;' .. package.path

@asmagill
Copy link
Member

I would be curious if a brand new build of 0.9.78 with the current XCode (12, I believe) has the same issue because I don't believe that the relevant code in MJLua.m and setup.lua (which set up the initial paths) has changed in a long time.

@latenitefilms
Copy link
Contributor

Ironically, I think @luckman212's fix for another problem actually introduced this particular issue?

@asmagill & @cmsj - I think the problem was introduced here: #2359

Specifically stringByStandardizingPath was changed to stringByResolvingSymlinksInPath here.

I'm not sure of the best solution to fix?

@latenitefilms
Copy link
Contributor

Regarding:

I searched the source to try and find where configdir gets initialized, but I'm just not seeing it. I see it's used in setup.lua#L3. Is this a Lua builtin or something?

setup.lua currently looks like:

local modpath, prettypath, fullpath, configdir, docstringspath, hasinitfile, autoload_extensions = ...
package.path=configdir.."/?.lua"..";"..configdir.."/?/init.lua"..";"..configdir.."/Spoons/?.spoon/init.lua"..";"..package.path..";"..modpath.."/?.lua"..";"..modpath.."/?/init.lua"
package.cpath=configdir.."/?.so"..";"..package.cpath..";"..modpath.."/?.so"

The configdir comes from MJLuaInit() in MJLua.m (see line 846):

lua_pushstring(L, [MJConfigDir() UTF8String]);

...and MJConfigDir() in defined in MJConfigUtils.m:

#import "MJConfigUtils.h"
#import "variables.h"

NSString* MJConfigDir(void) {
    return [MJConfigFileFullPath() stringByDeletingLastPathComponent];
}

NSString* MJConfigFileFullPath(void) {
    return [[MJConfigFile stringByStandardizingPath] stringByResolvingSymlinksInPath];
}

As explained in my last post, MJConfigFileFullPath has changed between releases, which I believe is the difference.

When I'm running Hammerspoon 0.9.80 (5530) I get:

> hs.configdir
/Users/chrishocking/.hammerspoon

> os.getenv('HOME') .. '/.hammerspoon'
/Users/chrishocking/.hammerspoon

> package.path
/Users/chrishocking/.hammerspoon/?.lua;/Users/chrishocking/.hammerspoon/?/init.lua;/Users/chrishocking/.hammerspoon/Spoons/?.spoon/init.lua;/usr/local/share/lua/5.4/?.lua;/usr/local/share/lua/5.4/?/init.lua;/usr/local/lib/lua/5.4/?.lua;/usr/local/lib/lua/5.4/?/init.lua;./?.lua;./?/init.lua;/Applications/Hammerspoon.app/Contents/Resources/extensions/?.lua;/Applications/Hammerspoon.app/Contents/Resources/extensions/?/init.lua

> package.cpath
/Users/chrishocking/.hammerspoon/?.so;/usr/local/lib/lua/5.4/?.so;/usr/local/lib/lua/5.4/loadall.so;./?.so;/Applications/Hammerspoon.app/Contents/Resources/extensions/?.so

When I'm running Hammerspoon 0.9.78 (5164) I get:

> hs.configdir
/Users/chrishocking/.hammerspoon

> os.getenv('HOME') .. '/.hammerspoon'
/Users/chrishocking/.hammerspoon

> package.path
/Users/chrishocking/.hammerspoon/?.lua;/Users/chrishocking/.hammerspoon/?/init.lua;/Users/chrishocking/.hammerspoon/Spoons/?.spoon/init.lua;/usr/local/share/lua/5.3/?.lua;/usr/local/share/lua/5.3/?/init.lua;/usr/local/lib/lua/5.3/?.lua;/usr/local/lib/lua/5.3/?/init.lua;./?.lua;./?/init.lua;/private/var/folders/tp/p7td6g9x28d425wct7ryyr_40000gn/T/AppTranslocation/71EAE9DB-CE95-4F94-9871-F1B9C09BCB76/d/Hammerspoon.app/Contents/Resources/extensions/?.lua;/private/var/folders/tp/p7td6g9x28d425wct7ryyr_40000gn/T/AppTranslocation/71EAE9DB-CE95-4F94-9871-F1B9C09BCB76/d/Hammerspoon.app/Contents/Resources/extensions/?/init.lua

> package.cpath
/Users/chrishocking/.hammerspoon/?.so;/usr/local/lib/lua/5.3/?.so;/usr/local/lib/lua/5.3/loadall.so;./?.so;/private/var/folders/tp/p7td6g9x28d425wct7ryyr_40000gn/T/AppTranslocation/71EAE9DB-CE95-4F94-9871-F1B9C09BCB76/d/Hammerspoon.app/Contents/Resources/extensions/?.so

...so these changes don't affect most users, but if you're sym-linking your init.lua file, I can see this would be a problem.

I'm not sure what the right fix is though?

@asmagill
Copy link
Member

@latenitefilms you are absolutely correct -- missed that line as it's in MJLuaConfigUtils.m and I didn't look there when comparing the two versions.

As #2359 was introduced to help with Spoon installation, the only real fix to regain past behavior of hs.configdir is to revert #2359, add new functions in MJConfigUtils.m for MJConfigDirand MJConfigFileFullPath with new names that resolve symlinks (i.e. use stringByResolvingSymlinksInPath), and then use those new functions in MJAppDelegate.m

That is if we're certain that:

  1. make MJConfigFile resolve symlinksΒ #2359 does in fact fix the issue it was created for, and
  2. The previous calculated value for hs.configdir is what we want going forward.

@latenitefilms
Copy link
Contributor

We probably don't need to alter hs.configdir - we could just add additional package.path and package.cpath values which use the original stringByStandardizingPath method - so both bases are covered?

@asmagill
Copy link
Member

I use hs.configdir in a number of places... since I'm not symlinking, I haven't observed any difference, but I think if you want both captured, we're better off reverting hs.configdir and adding hs.absoluteConfigdir (or something along those lines)... a few more lines of code to add, as we'll need to adjust the variables passed into setup.lua (and _coresetup/init.lua?), but not hard.

This would be in line with the shell command pwd -- by default it returns the "logical" path, but if you issue pwd -P, it issues the physical or absolute path.

@asmagill
Copy link
Member

FWIW, I think I prefer hs.configdirAbsolute or hs.configdirPhysical -- that way they're right next to each other in the docs.

@latenitefilms
Copy link
Contributor

Thinking about it, we could just revert to original behaviour, and to fix #2357 we could resolve the symlinks here instead - rather than touching hs.configdir or creating a new hs.configdirAbsolute?

@latenitefilms
Copy link
Contributor

For example:

NSString *configDir = MJConfigDir();
NSString *spoonPath = [[[configDir stringByStandardizingPath] stringByResolvingSymlinksInPath] stringByAppendingPathComponent:@"Spoons"];

@asmagill
Copy link
Member

$ egrep -n 'MJConfig\w+\(' MJAppDelegate.m
52:        NSString *spoonPath = [MJConfigDir() stringByAppendingPathComponent:@"Spoons"];
57:            NSLog(@"User double clicked on a Spoon in %@, skipping", MJConfigDir());
191:        NSString *spoonsPath = [MJConfigDir() stringByAppendingPathComponent:@"Spoons"];
212:    MJEnsureDirectoryExists(MJConfigDir());
213:    [[NSFileManager defaultManager] changeCurrentDirectoryPath:MJConfigDir()];
333:    NSString* path = MJConfigFileFullPath();

Looks like a couple of more places you might need to consider (not sure that all of them matter as much, but they should all be looked at closely to make sure), but still doable...

Does one of the additions you've made recently to hs.fs allow for converting a logical path into an absolute one? If not, we might want to add it if we go this route. Having brought up the distinction, I can imagine a future case where I might care... it happens occasionally in shell scripts I write, no reason to assume it couldn't in lua ones.

@latenitefilms
Copy link
Contributor

hs.fs.pathToAbsolute() does this, right? It returns:

  • A string containing the absolute path of filepath (i.e. one that doesn't intolve ., .. or symlinks)
  • Note that symlinks will be resolved to their target file

(I did just notice that typo too!)

I guess there's no harm in adding hs.configdirAbsolute() - so might as well do that as an extra option.

I'll leave this to you, unless you want me to tackle later today.

@asmagill
Copy link
Member

Yes, that functions does what I would need:

> hs.configdir .. "/Spoons"
/Users/xxxxx/.config/hammerspoon/Spoons

> hs.fs.pathToAbsolute(hs.configdir .. "/Spoons")
/opt/xxxxx/src/hammerspoon/Spoons/Source

My Spoons directory is a symlink into the source folder of the repository to make it easier to keep a current full set -- I haven't run into the issue with double clicking on them because I already have all of the standard ones, and I manually move any others into hs.configDir .. "/_Spoons" which I've added to my path and use for "in development" work.

No specific need for hs. hs.configdirAbsolute for me then, this function actually does more than adding a second string would.

So is the consensus to revert hs.configDir and manually adjust MJAppDelegate.m to perform the necessary conversion to absolute paths?

I can try to work up a pull tomorrow unless you want to do so sooner, either way is fine with me.

@latenitefilms
Copy link
Contributor

@asmagill & @luckman212 - I've pushed a fix in #2490, which I'm hoping solves both this issue (#2478), and the original #2357 issue. Any questions let me know.

@asmagill
Copy link
Member

Automatically closed when pull merged; reopen if you still have an issue after trying a build off of master branch.

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 a pull request may close this issue.

4 participants