CMake: Make BUILD_MODULE and BUILD_SHARED_LIBS non inclusive by doronbehar · Pull Request #459 · luvit/luv

doronbehar · 2020-02-26T17:16:18Z

Running out of patience and in order to get my point better, here's a change I'm satisfied with it's results, here is the resulting tree for all 3 configurations of ON and OFF:

prefix-build-module-off-shared-on
└── var
    └── empty
        └── local
            ├── include
            │   └── luv
            │       ├── lhandle.h
            │       ├── lreq.h
            │       ├── luv.h
            │       └── util.h
            └── lib
                ├── libluv.so
                ├── libluv.so.1 -> libluv.so.1.34.1
                ├── libluv.so.1.34.1
                └── pkgconfig
                    └── libluv.pc
prefix-build-module-on-shared-off
└── var
    └── empty
        └── local
            └── lib
                └── lua
                    └── 5.1
                        └── libluv.so
prefix-build-module-on-shared-on
└── var
    └── empty
        └── local
            ├── include
            │   └── luv
            │       ├── lhandle.h
            │       ├── lreq.h
            │       ├── luv.h
            │       └── util.h
            └── lib
                ├── libluv.so
                ├── libluv.so.1 -> libluv.so.1.34.1
                ├── libluv.so.1.34.1
                ├── lua
                │   └── 5.1
                │       └── libluv.so
                └── pkgconfig
                    └── libluv.pc

22 directories, 18 files

Fixes #457 and fixes NixOS/nixpkgs#80528 .

NOTE: I've had to create an additional target - libluv which is distinguished from the luv target.

doronbehar · 2020-02-26T17:48:13Z

cc @teto.

Fixes luvit#457 and fixes NixOS/nixpkgs#80528

CMakeLists.txt

squeek502 · 2020-02-27T00:43:03Z

Looks good, this is the direction I was thinking of going in as well. Looks like it needs some fixes to get LuaRocks builds working again--I'll work on that.

… type

zhaozg · 2020-02-28T10:17:08Z

Not late, This will break luvi build, we need do more work. @squeek502

squeek502 · 2020-02-29T02:19:41Z

Not late, This will break luvi build, we need do more work. @squeek502

Sorry, you're right. I jumped the gun on this one. luvi expects to be able to build a static library by turning BUILD_MODULE off, but it's no longer possible to build a static library since BUILD_SHARED_LIBS affects the default library type of add_library. This definitely needs some more work.

This adds on to the changes in luvit#459 to make it possible to build module, static, and shared libraries all at the same time if requested - To build the module, `-DBUILD_MODULE=On`; the target name for the module is `luv` - To build a static library, `-DBUILD_STATIC_LIBS=On`; the target name for the static lib is `libluv_a` (this matches how `libuv` names its targets; `uv` for shared and `uv_a` for static) - To build a shared library, `-DBUILD_SHARED_LIBS=On`; the target name for the shared lib is `libluv` These are able to be mixed/matched as needed. This is a breaking change (as was luvit#459) for users of CMake (e.g. luvi) who will now need to specify linking against `libluv`/`libluv_a` and turn on `BUILD_STATIC_LIBS`/`BUILD_SHARED_LIBS` before calling `add_subdirectory`

Updates CMake to be compatible with the changes in luvit/luv#459 and luvit/luv#461

teto · 2020-02-29T14:26:50Z

another issue we see in NixOS/nixpkgs#80528 (comment)
(the install FILES given no DESTINATION! part) is that SHAREDLIBS_INSTALL_INC_DIR definition is kinda hard to follow and lacks a good default. Once CMAKE_INSTALL_PREFIX is set can't we set SHAREDLIBS_INSTALL_INC_DIR from that or remove it altogether ?

squeek502 · 2020-03-01T00:00:10Z

@teto totally open to suggestions here. I'm not sure I fully understand what the issue is/what the possible fixes are.

doronbehar · 2020-03-01T15:16:25Z

@squeek502 Our build system calls luarocks make which in turn calls cmake with the following flags:

-G"Unix Makefiles"
-DCMAKE_MODULE_LINKER_FLAGS="-shared"
-DLIBDIR="/nix/store/5wf8zrgpp199pmbbgyvkp7hc484pzrbs-luajit-2.1.0-beta3-luv-1.34.2-0/luv-1.34.2-0-rocks/luv/1.34.2-0/lib"
-DLUA_INCDIR="/nix/store/1m8vh1byjnjmjkpdidcqc37cykgsqjap-luajit-2.1.0-beta3/include/luajit-2.1"
-DLUA_LIBDIR=""
-DLUA="/nix/store/1m8vh1byjnjmjkpdidcqc37cykgsqjap-luajit-2.1.0-beta3/bin/luajit"
-DCMAKE_LIBRARY_PATH="/nix/store/1m8vh1byjnjmjkpdidcqc37cykgsqjap-luajit-2.1.0-beta3/lib:/nix/store/w89ndqsb7ig22mw5hk7k7zfvxmhx44xb-libuv-1.34.2/lib:/nix/store/1m8vh1byjnjmjkpdidcqc37cykgsqjap-luajit-2.1.0-beta3/lib:/nix/store/w89ndqsb7ig22mw5hk7k7zfvxmhx44xb-libuv-1.34.2/lib"
-DCMAKE_INCLUDE_PATH="/nix/store/1m8vh1byjnjmjkpdidcqc37cykgsqjap-luajit-2.1.0-beta3/include:/nix/store/w89ndqsb7ig22mw5hk7k7zfvxmhx44xb-libuv-1.34.2/include:/nix/store/1m8vh1byjnjmjkpdidcqc37cykgsqjap-luajit-2.1.0-beta3/include:/nix/store/w89ndqsb7ig22mw5hk7k7zfvxmhx44xb-libuv-1.34.2/include"
-DLUADIR="/nix/store/5wf8zrgpp199pmbbgyvkp7hc484pzrbs-luajit-2.1.0-beta3-luv-1.34.2-0/luv-1.34"

Where /nix/store/5wf8zrgpp199pmbbgyvkp7hc484pzrbs-luajit-2.1.0-beta3-luv-1.34.2-0/ is our $prefix.

There are a couple of issues here which can be detected and handled somehow in CMakeLists.txt by detecting that LUA is set.

LIBDIR is set to $prefix/luv-1.34.2-0-rocks/luv/1.34.2-0/lib which is not where libluv.so should end up, this is where the file loaded by lua -e "require('lua')" should end up.
The cmake configure phase fails with the following error:

CMake Error at CMakeLists.txt:229 (install):
  install FILES given no DESTINATION!

I'm not sure my self how to address it yet, but this is my best summarise so I think it's a good start.

doronbehar · 2020-03-01T15:18:58Z

Oh actually, there's even 1 more thing I was thinking of, but consider this one as a nice-to-have:

Since luarocks/luarocks#1160, luarocks/luarocks#509 and luarocks/luarocks#339 are unresolved, perhaps would it be possible to set cmake options via environmental variables? This would help us make the build even cleaner.

teto · 2020-03-01T15:20:32Z

basically we should avoid the scenario

CMake Error at CMakeLists.txt:229 (install):
  install FILES given no DESTINATION!

if we follow the definitions, it goes up to SET(INSTALL_LIB_DIR ${LIBDIR}) but LIBDIR is never defined ?

doronbehar · 2020-03-01T15:23:46Z

if we follow the definitions, it goes up to SET(INSTALL_LIB_DIR ${LIBDIR}) but LIBDIR is never defined ?

LIBDIR is defined, but not to the value we would want it to have.

teto · 2020-03-01T15:51:07Z

where is it defined ? is that some kind of default ? I only can find LUA_LIBDIR.

doronbehar · 2020-03-01T16:10:22Z

It seems like luarocks default, see:

https://github.com/luarocks/luarocks/blob/ec4c5afe2cb70a5ca3e19a5e75d3561316727385/src/luarocks/rockspecs.lua#L63-L80

And:

https://github.com/luarocks/luarocks/blob/ec4c5afe2cb70a5ca3e19a5e75d3561316727385/src/luarocks/path.lua#L108-L119

This adds on to the changes in luvit#459 to make it possible to build module, static, and shared libraries all at the same time if requested - To build the module, `-DBUILD_MODULE=On`; the target name for the module is `luv` - To build a static library, `-DBUILD_STATIC_LIBS=On`; the target name for the static lib is `libluv_a` (this matches how `libuv` names its targets; `uv` for shared and `uv_a` for static) - To build a shared library, `-DBUILD_SHARED_LIBS=On`; the target name for the shared lib is `libluv` These are able to be mixed/matched as needed. This is a breaking change (as was luvit#459) for users of CMake (e.g. luvi) who will now need to specify linking against `libluv`/`libluv_a` and turn on `BUILD_STATIC_LIBS`/`BUILD_SHARED_LIBS` before calling `add_subdirectory`

This comment has been minimized.

Sign in to view

doronbehar force-pushed the cmake-fix branch from 87c3d35 to b023d9c Compare February 26, 2020 17:45

This comment has been minimized.

Sign in to view

CMake: Make BUILD_MODULE and BUILD_SHARED_LIBS non inclusive

0494414

Fixes luvit#457 and fixes NixOS/nixpkgs#80528

doronbehar force-pushed the cmake-fix branch from b023d9c to 0494414 Compare February 26, 2020 17:50

doronbehar commented Feb 26, 2020

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

squeek502 added 2 commits February 26, 2020 16:57

CMake: Fix install paths when building from LuaRocks

77112b6

CMake: Reduce redundant target_link_libraries setup code for each lib…

5081110

… type

squeek502 approved these changes Feb 27, 2020

View reviewed changes

squeek502 merged commit f3fe11f into luvit:master Feb 27, 2020

squeek502 mentioned this pull request Feb 29, 2020

CMake: Add BUILD_STATIC_LIBS option #461

Merged

squeek502 added a commit to squeek502/luvi that referenced this pull request Feb 29, 2020

Update to luv 1.34.2-1

0ad030c

Updates CMake to be compatible with the changes in luvit/luv#459 and luvit/luv#461

squeek502 added a commit to squeek502/luvi that referenced this pull request Feb 29, 2020

Update to luv 1.34.2-1

073c5b1

Updates CMake to be compatible with the changes in luvit/luv#459 and luvit/luv#461

squeek502 mentioned this pull request Feb 29, 2020

Update to luv 1.34.2-1 luvit/luvi#228

Merged

doronbehar mentioned this pull request Mar 7, 2020

luaPackages.luv: cleanup build NixOS/nixpkgs#80528

Closed

10 tasks

doronbehar mentioned this pull request Aug 13, 2021

luarocks: 3.2.1 -> 3.7.0 NixOS/nixpkgs#133553

Closed

11 tasks

Conversation

doronbehar commented Feb 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

doronbehar commented Feb 26, 2020

Uh oh!

Uh oh!

squeek502 commented Feb 27, 2020

Uh oh!

zhaozg commented Feb 28, 2020

Uh oh!

squeek502 commented Feb 29, 2020

Uh oh!

teto commented Feb 29, 2020

Uh oh!

squeek502 commented Mar 1, 2020

Uh oh!

doronbehar commented Mar 1, 2020

Uh oh!

doronbehar commented Mar 1, 2020

Uh oh!

teto commented Mar 1, 2020

Uh oh!

doronbehar commented Mar 1, 2020

Uh oh!

teto commented Mar 1, 2020

Uh oh!

doronbehar commented Mar 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

doronbehar commented Feb 26, 2020 •

edited

Loading