Win64 installation by diz-vara · Pull Request #10 · torch/luajit-rocks

diz-vara · 2014-12-11T11:32:37Z

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 · 2014-12-11T15:40:13Z

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 · 2014-12-11T15:49:59Z

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 notifications@github.com
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).

points to my repository

my server in readme.md

…o july

Ark-kun · 2016-05-04T09:27:21Z

luajit-2.0/src/lua.h

+# 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") 


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

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

@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

hughperkins · 2016-09-17T15:19:53Z

luarocks/src/luarocks/config.lua.win.in

 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]],


Not sure we want this in official torch distribution?

@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

hughperkins · 2016-09-17T15:20:25Z

README.md

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


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

hughperkins · 2016-09-17T15:20:36Z

.gitignore

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


you can do simply:

build/

hughperkins · 2016-09-17T15:21:08Z

.gitignore

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


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

hughperkins · 2016-09-17T15:21:48Z

README.md

+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 :


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

hughperkins · 2016-09-17T15:23:03Z

lua-5.1/src/lua.h

+# ifndef LUA_WIN
+#  define LUA_WIN
+# endif
+# ifndef _WIN32


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

hughperkins · 2016-09-17T15:25:27Z

luajit-2.1/src/luaconf.h

 #endif
 #else
-#define LUA_API		extern
+#define LUA_API		extern "C"


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

hughperkins · 2016-09-17T15:25:53Z

luarocks/src/luarocks/build/builtin.lua

         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))


can we remove the dead code?

hughperkins · 2016-09-17T15:26:35Z

luarocks/src/luarocks/config.lua.in

 }

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


Prefer to remove this, for torch distribution

hughperkins · 2016-09-17T17:41:48Z

@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.

diz-vara added 10 commits December 8, 2014 16:07

ignore build directory

897c041

tools -> bin; chmod, rm and objdump added

3add215

x64 configurations

26710cb

cmake.cmd added

e9a2252

x64 scripts

807e9b5

simple path to binary Win32 tools

17a963c

cmake added to the list of files to copy

9428013

cmake added to the list of fies to copy

1b277d7

WIN32: additional defines and libluajit import for different rocks

21e8bb4

pause removed from cmake

8f723a6

resolved x64 compilation problems

921ae9f

soumith mentioned this pull request Feb 11, 2015

Torch on windows torch/torch7#150

Closed

diz-vara and others added 6 commits February 12, 2015 20:56

Update config.lua.win.in

77aa68e

points to my repository

Update config.lua.in

d1f2496

points to my repository

ignore build files

7e7c301

Merge https://github.com/diz-vara/luajit-rocks into july

afafb03

Merge remote-tracking branch 'upstream/master' into july

f8f9e22

allow original servers

1c4179b

diz-vara force-pushed the master branch from 6a973dd to 1c4179b Compare July 14, 2015 07:53

diz-vara and others added 4 commits July 17, 2015 10:50

Update README.md

74a7f44

Update README.md

9f53d34

missed ',' in server list

298f505

my server in readme.md

Merge branch 'master' of https://github.com/diz-vara/luajit-rocks int…

fd35804

…o july

Ark-kun reviewed May 4, 2016
View reviewed changes

hughperkins reviewed Sep 17, 2016

View reviewed changes

.gitignore

@@ -0,0 +1,38 @@

build/*.*

Copy link

hughperkins Sep 17, 2016

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/

hughperkins reviewed Sep 17, 2016

View reviewed changes

Conversation

diz-vara commented Dec 11, 2014

Uh oh!

soumith commented Dec 11, 2014

Uh oh!

diz-vara commented Dec 11, 2014

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!

hughperkins Sep 17, 2016 • 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!

hughperkins commented Sep 17, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hughperkins Sep 17, 2016 •

edited

Loading