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

Win installer: Unversion LR target path #413

Merged
merged 10 commits into from
Nov 8, 2015

Conversation

Tieske
Copy link
Contributor

@Tieske Tieske commented Jul 18, 2015

This PR fixes the installer to use a non-versioned path for the LR installation (fixes #151). As discussed here #403 (comment) . It also fixes the retainment of the existing config files.

One problem though; because the LR path is moving one level up, the system rock tree is now in the same path. e.g. default used to be c:\program files (x86)\luarocks\2.2 and now is c:\program files (x86)\luarocks. This means that when deleting that existing directory upon installation, the default location of the system tree; c:\program files (x86)\luarocks\systree, is now also deleted.

iirc correctly the system tree used to be installed in the Lua directories, not the LuaRocks directories. Some 2 years ago this was changed, but with hind sight that seems a bad change, so maybe that should be reverted.

For now; this is for discussion only, do not merge yet, until the systree location issue is resolved.

@Tieske
Copy link
Contributor Author

Tieske commented Jul 19, 2015

@ignacio any idea why LR refuses to run, complaining that the current directory doesn't exist? The added debug line "cd && dir" gives proper output...

I had a quick look at the source in fs\win32\tools.lua, but I don't understand this 'directory stack' concept that seems to be used? what is it for, and how does it work?

@ignacio
Copy link
Contributor

ignacio commented Jul 20, 2015

Hi @Tieske . Will take a look at this later today and get back to you.

@Tieske
Copy link
Contributor Author

Tieske commented Jul 20, 2015

It's already fixed. I thought it to be an appveyor issue, but it turned out to be an issue with cfg.Lua.

So back to the original issue; location of the system tree... Any ideas?

@hishamhm
Copy link
Member

Perhaps bring the issue to the LR mailing list to see if any other Windows users chime in? I trust you guys to make the final call, but it would be nice to at least hear what users have to say on this matter.

@ignacio
Copy link
Contributor

ignacio commented Jul 20, 2015

I think of two options.

  1. Even when the versioned directory is removed (e.g. 2.2), still leave a fixed one. So instead of c:\program files (x86)\luarocks\2.2 we get c:\program files (x86)\luarocks\install or something like that.

  2. Move the system tree to a different place. Local AppData seems like a reasonable place. I'm pretty sure this has been discussed already. I can't recall the pros and cons of each location.

@siffiejoe
Copy link
Contributor

If I understand correctly, the current issue is that a rm -rf $LUAROCKS_PREFIX also removes the system rocks tree, because it is now inside of $LUAROCKS_PREFIX. If so, another obvious option would be to not do the rm -rf $LUAROCKS_PREFIX. Either remove the necessary files/directories explicitly, or leave them be. If the installer is used to install additional versions of LuaRocks in the same directory, you shouldn't remove the old installation anyway.

@Tieske
Copy link
Contributor Author

Tieske commented Jul 20, 2015

Move the system tree to a different place. Local AppData seems like a reasonable place. I'm pretty sure this has been discussed already. I can't recall the pros and cons of each location.

iirc it was for security reasons, LR data is not real "data" but application code, and as such needs admin priviledges to write to, hence the Program Files location, as AppData is not protected in that way.

If I understand correctly

@siffiejoe you did

If so, another obvious option would be to not do the rm -rf $LUAROCKS_PREFIX. Either remove the necessary files/directories explicitly

Not the cleanest solution imo, also very vulnerable to upgrade mistakes if files are moved/added/renamed. Due to the versioned directory they were separated before, my preference would be to keep them separated

If the installer is used to install additional versions of LuaRocks in the same directory, you shouldn't remove the old installation anyway.

I don't see how this could work? The other discussion was regarding one LR installation serving rocks for multiple Lua installations, maybe that is what you meant? This pull request takes care of retaining the config[-x.x].lua and site_config[_x_x.lua] files in that case (copied to a temp location before removing the full directory, and copied back after the main files have been installed)

@hishamhm
Copy link
Member

  1. Even when the versioned directory is removed (e.g. 2.2), still leave a fixed one. So instead of c:\program files (x86)\luarocks\2.2 we get c:\program files (x86)\luarocks\install or something like that.

I thought of that option too.

Alternatively, how about this?

  • "c:\Program Files (x86)\LuaRocks.org\LuaRocks"
  • "c:\Program Files (x86)\LuaRocks.org\Installed Rocks"

Looks very Windows-y to me ;)

@Tieske
Copy link
Contributor Author

Tieske commented Jul 22, 2015

I like the LuaRocks.org namespacing. 👍

Alternatively I was considering using the Lua tree;

{root}
  +-- bin
  +-- include
  +-- lib
  |    +-- lua
  |         +-- <LuaVersion>
  +-- man
  |    +-- man1
  +-- share
       +-- lua
            +-- <LuaVersion>

This is the default structure of the makefile for MinGW. And Lua 5.3 includes the defaults paths for these as well (LuaWinMake also creates this structure when installing).
So if one would use the {root} above as the systemtree, then no Lua path environment variables need to be set at all. Everything would work out of the box, on defaults.

as a bonus it would be versioned (except for bin).

@Tieske
Copy link
Contributor Author

Tieske commented Jul 22, 2015

we could even place a small luarocks.bat wrapper batch file in the bin directory, so only adding the path to lua.exe to the system path would make it all work, including the LuaRocks commandline.

@hishamhm
Copy link
Member

Alternatively I was considering using the Lua tree;

Yeah, that would be the Unix-y solution. I think that's what I wanted to go with in the earliest days (due to sheer laziness/simplicity of having everything look/work the same everywhere) but for some reason we didn't go with that (Kepler already had different conventions, maybe? Or maybe Windows people were offended by the Unix-like paths, I don't remember :) )

@Tieske
Copy link
Contributor Author

Tieske commented Jul 22, 2015

I think things have changed, the tree structure has been standardized a little bit more by Lua 5.3 including the defaults paths for this structure. And the earlier mentioned security, previously everyone on Windows had admin rights, and that has also changed (for the better).

If LR adopts this structure, I think the 'standard' would become a little stronger again...

@hishamhm
Copy link
Member

Well, you certainly have my support for the unified bin+lib+share structure in all OSes :)

@ignacio
Copy link
Contributor

ignacio commented Jul 22, 2015

So, what would {root} be? I didn't get that part. Could you provide some examples with full paths?

@Tieske
Copy link
Contributor Author

Tieske commented Jul 22, 2015

{root} would be the root of the Lua installation. On my system that would be C:\Program Files (x86)\Lua.
So the full tree of my Lua installation is;

C:\Program Files (x86)
    +-- Lua   
        +-- bin
        +-- include
        +-- lib
        |    +-- lua
        |         +-- 5.1
        +-- man
        |    +-- man1
        +-- share
             +-- lua
                  +-- 5.1

At the same time my system tree is;

C:\Program Files (x86)
    +-- LuaRocks
        +-- systree
            +-- bin
            +-- lib
            |    +-- lua
            |    |    +-- 5.1
            |    +-- luarocks
            |         +-- rocks
            +-- share
                 +-- lua
                      +-- 5.1

So if I would point my system tree to C:\Program Files (x86)\Lua they would exactly match on top of each other. And in case of Lua 5.3 I wouldn't even have to change the Lua environment variables to find the installed rocks, as they are already in the defaults.

@ignacio
Copy link
Contributor

ignacio commented Jul 22, 2015

So you'd be install the system tree in C:\Program Files (x86)\Lua. Great.
And LuaRocks own files would live in c:\Program Files (x86)\LuaRocks and not in C:\Program Files (x86)\Lua, right?

@Tieske
Copy link
Contributor Author

Tieske commented Jul 22, 2015

Yes, just the system rocktree would be integrated into the Lua directory tree, LuaRocks would still have its own, non-versioned, installation directory.

@ignacio
Copy link
Contributor

ignacio commented Jul 22, 2015

Great. I'm ok with this.

@Tieske
Copy link
Contributor Author

Tieske commented Jul 26, 2015

Just updated this PR with the change as discussed. Please give this a try and let me know whether it is good to go.

@hishamhm
Copy link
Member

Sorry about chiming in late, but... changing the tree to a Unix-like structure is good, but I'm unsure about our installer installing Lua at C:\Program Files (x86)\Lua. Looking at this path, it looks like something straight from lua.org. Shouldn't we either use something namespaced like C:\Program Files (x86)\LuaRocks.org\LuaRocks and C:\Program Files (x86)\LuaRocks.org\Lua, to avoid any possible clashes with other Lua installations? (I understand that the latter is also misleading as it looks like Lua comes from LuaRocks.org, but at least that's true in the sense that this is the build we're shipping).

OR...

While we're at it... why two trees? If the LR system tree will store stuff in Lua's tree anyway, why not install LuaRocks itself there too? In other words...

c:\Program Files (x86)\
   LuaRocks
      bin
         lua-5.1
         luarocks-5.1
         luarocks
      lib
         lua
            5.1
      share
         lua
            5.1

Removing a LuaRocks version when upgrading equates to deleting bin\luarocks*.* and share\lua\5.1\luarocks, assuming the configuration stays the same... it shouldn't be messy, right?

@hishamhm
Copy link
Member

Also, this will be a major usability change for users... maybe we should make this move in the luarocks-3 branch?

@Tieske
Copy link
Contributor Author

Tieske commented Jul 27, 2015

Sorry about chiming in late, but... changing the tree to a Unix-like structure is good, but I'm unsure about our installer installing Lua at C:\Program Files (x86)\Lua. Looking at this path, it looks like something straight from lua.org.

The bundled Lua version is installed as C:\Program Files (x86)\LuaRocks\lua5.1.exe, so inside the LR directory. That example was just my own Lua installation, so it wasn't installed by the LR installer. Sorry for the confusion.

While we're at it... why two trees? If the LR system tree will store stuff in Lua's tree anyway, why not install LuaRocks itself there too?

I'm not sure about this one. I see your point, but I'm not comfortable with it (yet). Have to give it some thought. Maybe take it one step at a time. And make that a next step.
Maybe @ignacio or @siffiejoe have some thoughts on this?

Also, this will be a major usability change for users... maybe we should make this move in the luarocks-3 branch?

No, I don't think so. Last time we moved the tree, it also was a point release. A user just needs the /TREE argument to move the tree elsewhere, or he doesn't move it and just reinstalls the modules he wants/needs. Imho adding a note to the release should suffice.

@siffiejoe
Copy link
Contributor

Maybe @ignacio or @siffiejoe have some thoughts on this?

To clarify: We are talking about four directory trees. Two are also rocks trees (one requires admin rights, the other doesn't). Then there is the Lua installation directory, and the LuaRocks installation directory.

The current plan is to put one of the rocks trees into the Lua installation directory, because then Lua 5.3, when installed using a certain directory layout, doesn't need any LUA_PATH* variables set to access the modules. Correct? I'm a bit uncomfortable installing LuaRocks into a directory owned by another program, but ok. But then the LuaRocks batch files should be installed side-by-side with lua.exe so that you also don't have to modify the PATH variable (if you have already done so for Lua). (The various tools shouldn't be in PATH, so I'd put them in .../lib/luarocks/tools or something.)

Another idea was to move the LuaRocks modules from the LuaRocks installation directory into the system rocks tree. This is already done on Unix, and it will happen anyway as soon as we support updating LuaRocks via rockspec on Windows, so I'm all for it. (Btw., what is holding us back there?)

If all of this is realized, we only have two directory trees left: One Lua installation directory which includes the system rocks tree and the LuaRocks installation, and one separate user rocks tree. This is exactly like on Unix but with some additional subdirectories in .../lib/luarocks (tools, icons, etc.).

@ignacio
Copy link
Contributor

ignacio commented Jul 27, 2015

(The various tools shouldn't be in PATH, so I'd put them in .../lib/luarocks/tools or something.)

Yes, they must not be in the path. Care is taken to address them using absolute paths so a LR install won't mess with my libeay32.dll and so on.

This is already done on Unix, and it will happen anyway as soon as we support updating LuaRocks via rockspec on Windows, so I'm all for it. (Btw., what is holding us back there?)

IIRC, the trouble is replacing files while in use. This issue mentions it.

@siffiejoe
Copy link
Contributor

IIRC, the trouble is replacing files while in use. This issue mentions it.

Yes, but that should not apply to Lua modules since they are opened, parsed, compiled, and then closed. So when LuaRocks code is executed there shouldn't be any Lua files in use. Is it the batch files that are the problem? I'll try to dig up the original source of that info ...

@ignacio
Copy link
Contributor

ignacio commented Jul 28, 2015

I think the problem was related to C modules which LR can use if available (LuaSocket, LuaSec, etc). Maybe there is no actual problem upgrading LuaRocks itself.

@hishamhm
Copy link
Member

I think the problem was related to C modules which LR can use if available (LuaSocket, LuaSec, etc).

Yes, that's why we have use_fs_modules = false on Windows now.

Maybe there is no actual problem upgrading LuaRocks itself.

Yes, maybe there isn't, and to be honest I just never tested it.

@hishamhm
Copy link
Member

Note however that for self-upgrade to work the first install has to be different, so that the LuaRocks files show up in the manifest. On Unix that required a special rockspec and makefile changes, for which I never implemented equivalents on Windows.

@Tieske
Copy link
Contributor Author

Tieske commented Jul 29, 2015

I'm a bit uncomfortable installing LuaRocks into a directory owned by another program, but ok.

Conceptually this is ok I think. LuaRocks is just a tool that helps extend the Lua environment, hence it has to modify it. So a rockstree is part of a Lua installation, but managed by LuaRocks.

Another idea was to move the LuaRocks modules from the LuaRocks installation directory into the system rocks tree. This is already done on Unix, and it will happen anyway as soon as we support updating LuaRocks via rockspec on Windows, so I'm all for it. (Btw., what is holding us back there?)

Here it is different imo. LuaRocks is a different application, one could consider it just a coincidence that it is written in Lua itself. But still it is a different application. Hence my hesitation. But reducing platform differences might be worth it.

But then the LuaRocks batch files should be installed side-by-side with lua.exe so that you also don't have to modify the PATH variable (if you have already done so for Lua). (The various tools shouldn't be in PATH, so I'd put them in .../lib/luarocks/tools or something.)

As an alternative; see this commit, it adds a "shortcut" batch file to the bin directory to access LR itself. And hence the LR installation remains separate from the Lua installation. Just a thought...

@Tieske
Copy link
Contributor Author

Tieske commented Jul 29, 2015

as for updating LuaRocks by itself; there are also the included binaries, and batchfiles. I'm not worried about the binaries, but the batchfiles might be an issue.

The way I understand they work is by executing a line, if the batch file is changed/deleted, the next line is read, but might be different because of a change. Probably if multiple statements are within () they are considered one, even if on multipe lines, so that makes it even less predictable.

I think workarounds can be devised, but it does require some care.

@Tieske
Copy link
Contributor Author

Tieske commented Jul 29, 2015

for now; should we merge this PR and open a new issue for making LR update itself on windows?

@Tieske
Copy link
Contributor Author

Tieske commented Oct 27, 2015

updated to warn if an environment variable is given, but the file referred to isn't available.

@Tieske
Copy link
Contributor Author

Tieske commented Nov 7, 2015

@hishamhm Can this be merged or is there still an issue left?

PS. I think we need a different branching model if developing in parallel for LR 3 and LR 2. Something like a development branch, for which all enhancements will be merged in both the master branch (LR 2.x) and in the LR 3 branch. New LR 3 features are only to be merged in the LR 3 branch.

@hishamhm
Copy link
Member

hishamhm commented Nov 8, 2015

@Tieske Yes, I believe this can be merged.

As for branches, I've been remiss and committed stuff into luarocks-3 that should have gone into master instead... Maybe we should think a bit more about the branching model, yes.

hishamhm added a commit that referenced this pull request Nov 8, 2015
Win installer: Unversion LR target path
@hishamhm hishamhm merged commit ce3ea55 into luarocks:master Nov 8, 2015
@Tieske Tieske deleted the unversion_LR branch November 10, 2015 21:10
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 this pull request may close these issues.

upgrade breaks installed batch files
4 participants