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

Win64 installation #10

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

Win64 installation #10

wants to merge 21 commits into from

Conversation

diz-vara
Copy link

Several improvements for win64 installation:

  • additional binary utils for luarocks/fs
  • path to binary utils changed
  • x64 platform added
  • cmake.cmd to add -G "NMake Makefiles" to cmake command line
  • additional defines (_WIN32, _WINDOWS) and libluajit import for different rocks to compile

tested with VS2013 x64 and rocks:
without any changes
paths
cwrap
torch
sys
nn
qtlua
xlua
with ';' changed to '&&' to separate build commands
optim
json
unsup
dp
gm

@soumith
Copy link
Member

soumith commented Dec 11, 2014

looks good to me.
are you planning to upstream these changes to luajit mainline, as well as luarocks mainline? (or are they already there?)

@andresy can you review and merge?

@diz-vara
Copy link
Author

I made these changed to this project only - and tested only on one system.
I plan to add changes to other rocks (image, qttorch, pprint, gnuplot -
they work on my system).
I'll think on luajit and luarocks mainlines - but not right now

On Thu, Dec 11, 2014 at 6:40 PM, Soumith Chintala [email protected]
wrote:

looks good to me.
are you planning to upstream these changes to luajit mainline, as well as
luarocks mainline? (or are they already there?)

@andresy https://github.com/andresy can you review and merge?


Reply to this email directly or view it on GitHub
#10 (comment).

# endif

# if !defined LUA_LIB & !defined LUA_CORE & !defined luajit_c & !defined _LJ_ARCH_H & !defined _LJ_DEF_H & !defined _LJ_OBJ_H
# pragma comment( lib, "libluajit.lib")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to use the linker options instead of #pragma.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I odnt know what this pragma does, but concur with the comment by Ark-kun, if that's indeed what this does.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hughperkins this pragma just links with the library - but only in certain cases (if module is not a part of the 'core' itself). It can be moved to the options - if somebody will implement these conditions in CMake

@@ -12,8 +12,9 @@ rocks_trees = {
}

rocks_servers = {
[[https://raw.githubusercontent.com/torch/rocks/master]],
[[https://raw.githubusercontent.com/rocks-moonscript-org/moonrocks-mirror/master]]
[[https://raw.githubusercontent.com/diz-vara/rocks/master]],

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure we want this in official torch distribution?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hughperkins Not for official, certainly! I keep my repo in the list for some windows-specific changes (eg in luaffifb, imgraph).
It would be OK to remove it - but we should check every rock in the official repo

```sh
git clone https://github.com/torch/luajit-rocks.git
git clone https://github.com/diz-vara/luajit-rocks.git

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For purposes of PR, I think this should point to torch, rather than diz-vara.

@@ -0,0 +1,38 @@
build/*.*

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can do simply:

build/

build/luarocks/src/luarocks/site_config.lua
build/Makefile
build/LUA_DEV
bld07/*

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bld07/ is non-standard :-) fine with it personally, but I reckon it'd be cleaner to remove it, on the hwole

cmake -DCMAKE_INSTALL_PREFIX=/your/prefix -DWITH_LUAJIT21=ON -G "NMake Makefiles" -DWIN32=1 -P cmake_install.cmake
```

Under Windows - remember to update your environment variables. Assuming that your/prefix is d:/luainstall :
Copy link

@hughperkins hughperkins Sep 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we make this a variable, rahter than d:/luatinstall. eg %LUA_INSTALL%

# ifndef LUA_WIN
# define LUA_WIN
# endif
# ifndef _WIN32

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be cleaner to not mess wit hthe lua sourcecode I reckon? I think I'd prefer these in our CMakeLists perhaps?

@@ -135,7 +138,7 @@
#define LUA_API __declspec(dllimport)
#endif
#else
#define LUA_API extern
#define LUA_API extern "C"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont think we should be messing around wit hthe original lua source code? We can simply add extern "C" in our own c++ header files?

#ifdef __cplusplus
extern "C"
#endif
    #include "luaconf.h"
#ifdef __cplusplus
}
#endif

@@ -116,7 +116,8 @@ function builtin.run(rockspec)
def:write("EXPORTS\n")
def:write("luaopen_"..name:gsub("%.", "_").."\n")
def:close()
local ok = execute(variables.LD, "-dll", "-def:"..deffile, "-out:"..library, dir.path(variables.LUA_LIBDIR, variables.LUALIB), unpack(extras))
-- diz --- local ok = execute(variables.LD, "-dll", "-def:"..deffile, "-out:"..library, dir.path(variables.LUA_LIBDIR, variables.LUALIB), unpack(extras))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we remove the dead code?

@@ -12,6 +12,7 @@ rocks_trees = {
}

rocks_servers = {
[[https://raw.githubusercontent.com/diz-vara/rocks/master]],

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer to remove this, for torch distribution

@hughperkins
Copy link

@diz-vara I've added comments to this PR, because I think I'd like to see this PR merged, and I reckon if we clean up the bits I've commented on, we can make a stronger case to Soumith to merge it.

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.

4 participants