search

martanne / vis

A vi-like editor based on Plan 9's structural regular expressions
Other
4.25k stars 257 forks source link

Build system overhaul #195

Closed lanodan closed 8 years ago

lanodan commented 8 years ago

Here is what I’ve changed to make it compile(without CONFIG_LUA=0 and custom prefix).

diff --git a/config.mk b/config.mk
index 193881a..0059cf3 100644
--- a/config.mk
+++ b/config.mk
@@ -22,6 +22,7 @@ LDFLAGS_CURSES = $(shell pkg-config --libs ncursesw 2> /dev/null || echo "-lncur

 ifeq (${CONFIG_LUA},1)
        CFLAGS_LUA = $(shell pkg-config --cflags lua5.2 2> /dev/null || echo "-I/usr/include/lua5.2")
+       CFLAGS_LUA += -DLUA_COMPAT_5_1 -DLUA_COMPAT_5_2 -DLUA_COMPAT_ALL
        LDFLAGS_LUA = $(shell pkg-config --libs lua5.2 2> /dev/null || echo "-llua -lm")
 endif

@@ -54,7 +55,7 @@ endif
 CFLAGS_LIBS = $(CFLAGS_LUA) $(CFLAGS_TERMKEY) $(CFLAGS_CURSES)
 LDFLAGS_LIBS = $(LDFLAGS_LUA) $(LDFLAGS_TERMKEY) $(LDFLAGS_CURSES) $(LIBS)

-CFLAGS_VIS = $(CFLAGS_LIBS) -std=c99 -Os -DVERSION=\"${VERSION}\" -DNDEBUG -D_POSIX_C_SOURCE=200809L -D_XOPEN_SOURCE=700 -DLUA_COMPAT_5_1 -DLUA_COMPAT_5_2 -DLUA_COMPAT_ALL
+CFLAGS_VIS = $(CFLAGS_LIBS) -std=c99 -Os -DVERSION=\"${VERSION}\" -DNDEBUG -D_POSIX_C_SOURCE=200809L -D_XOPEN_SOURCE=700
 LDFLAGS_VIS = $(LDFLAGS_LIBS)

 DEBUG_CFLAGS_VIS = ${CFLAGS_VIS} -UNDEBUG -O0 -g -ggdb -Wall -Wextra -pedantic -Wno-missing-field-initializers -Wno-unused-parameter
@@ -68,5 +69,7 @@ ifeq (${CC},gcc)
        LDFLAGS += -z now -z relro -pie
 else ifeq (${CC},clang)
        CFLAGS += -fPIE -fstack-protector-all -D_FORTIFY_SOURCE=2
-       LDFLAGS += -z now -z relro -pie
+       # -z now -z relro
+       # clang: error: No such file or directory
+       LDFLAGS += -pie
 endif
martanne commented 8 years ago

Is -z now -z relro in general not supported on FreeBSD?

If we wanted to be user friendly then adding a simple handwritten (certainly not autohell based) configure script probing for those compiler specific flags would probably make sense.

On the other hand these hardening flags only really make sense if they are applied system wide hence we could argue that users/distributions should add them to their regular CFLAGS if they care about them ...

tony commented 8 years ago

On FreeBSD too with similar problem and have been digging through config.mk too, but concerned about playing whackamole with the flags.

So autotools / CMake is out of the question?

martanne commented 8 years ago

So autotools / CMake is out of the question?

Yes. I know the current build system is not optimal, but adding autotools/Cmake complexity won't improve it.

The musl configure script is a good example of how a handwritten configure script should look like.

tony commented 8 years ago

autotools and CMake weren't one of the pain points in vim/neovim.

musl won't work with pmake. CMake would give me a POSIX Make compatible makefile or even ninja (if I have it installed and -GNinja). For musl I had to use GNU Make (not installed on by default on FreeBSD) to build.

Configuration is complex problem. When you do the math, they end up increasing portability, prevent reinventing the wheel and lower the barrier of entry for contributors since they're common. Most linux distros, BSD's and brew have autotools and cmake in the package repositories. I don't think they're doing it because they're foolish.

I have a totally difference in experience. I bit the bullet to learn CMake because projects I wanted to contribute to used it - I ended up finding it to be versatile, efficient and able to find libraries and build across 3 machines (Linux distro, OS X and FreeBSD). I started using it on new projects as well. This doesn't mean I don't experience pains getting builds to work across systems/library versions - you're going to go through hell finding libraries one way or another.

That said, CMake was far easier for me to make sense of than autotools. Autotools still seems like a jungle to me, and its a real barrier to me getting my hands dirty with some projects. So I'm forced to learn it.

Let's say we do musl configure style, could it make a POSIX Make compatible Makefile?

lanodan commented 8 years ago

I think a musl-style configure script rocks, but the ./configure should not make a Makefile but create a config.mk sourced in the Makefile (So the ./configure have less things to do). Also a ./configure would allow to use BSD and GNU make and to use it in another projects easily.

"Most linux distros, BSD's and brew have autotools and cmake in the package repositories." I think they do it because I would be foolish and a pain to maintain and make another build system just because you don't want theses.

Also CMake and autools have problems with cross-platforms too and cross-compiling with theses automagic tools is a huge mess.

martanne commented 8 years ago

Let's say we do musl configure style, could it make a POSIX Make compatible Makefile?

As @lanodan mentioned the idea would be to dynamically generate a few variable assignments for config.mk. This would eliminate the need for non-portable if statements within the main Makefile.

The make local and make standalone functionality uses some GNU make extensions and I have no plans to change/rewrite that. We could try to move these targets to a GNUmakefile (it is checked first by GNU make) which would include the standard and hopefully portable Makefile.

Anyway I currently do not have time to perform the necessary cross platform testing required for these changes. Therefore I have for now removed the hardening flags. Does it now build by default under FreeBSD? Feel free to provide patches to further tweak the build system ...

martanne commented 8 years ago

Did somebody start with a build system overhaul? A release is overdue and In principle it would be nice to include these changes because then they would get some testing exposure ...

martanne commented 8 years ago

I pushed a musl style configure script which generates config.mk. My shell scripting skills are a bit rusty, meaning it could probably be improved (for example there is quite some code duplication). However it should at least convey the basic idea. In theory it should be POSIX compatible and the resulting Makefile should work with both GNU and BSD make (not really tested).

The new build instructions are ./configure && make manually tweaking config.mk is still supported.

Testing would be welcome! Therefore I'm tagging various people who either recently contributed some patches or have shown past interest in the project (sorry for the spam).

Also I recently got a request for Windows support. Whether this should be done on top of midipix, Cygwin or MSYS2 is not yet clear, and will likely require some code changes.

eworm-de commented 8 years ago

Pushed a pull request #203... This is just about calling gcc with correct options (or options in correct order).

xomachine commented 8 years ago

Some build scenarios don't work correctly on linux due to absence of the SHAREPREFIX and MANPREFIX variables definitions in makefile as well as in generated config.mk.

eworm-de commented 8 years ago

Ah, right... Some pathes are borked... @xomachine, are you going to fix this?

eworm-de commented 8 years ago

Pushed PR #204...

martanne commented 8 years ago

Thanks, we should probably also support the --mandir and --datadir options to configure?

eworm-de commented 8 years ago

Just updated... ;) Though I used --sharedir. Is --datadir more common?

TieDyedDevil commented 8 years ago

No issues observed so far.

On Mon, Mar 21, 2016 at 04:52:54AM -0700, Marc André Tanner wrote:

I pushed a musl style configure script which generates config.mk. My shell scripting skills are a bit rusty, meaning it could probably be improved (for example there is quite some code duplication). However it should at least convey the basic idea. In theory it should be POSIX compatible and the resulting Makefile should work with both GNU and BSD make (not really tested).

The new build instructions are ./configure && make manually tweaking config.mk is still supported.

Testing would be welcome! Therefore I'm tagging various people who either recently contributed some patches or have shown past interest in the project (sorry for the spam). * [1]@eworm-de (Arch Linux) * [2]@TieDyedDevil (Fedora) * [3]@pullmoll (Void Linux) * @mreed1 (OpenBSD, Github account seems to be inactive/deleted) * [4]@lanodan, [5]@tony (FreeBSD) * [6]@iamleot (NetBSD) * [7]@rpmohn (AIX) * [8]@erf, [9]@twe4ked (Mac OS X) * [10]@xomachine * [11]@rgburke

rgburke commented 8 years ago

With pull request #206 applied vis builds successfully on Ubuntu 15.10.

I was able to build vis on Cygwin successfully (with compiler warnings though) by simply running PKG_CONFIG_PATH="/usr/local/lib/pkgconfig:$PKG_CONFIG_PATH" ./configure.

joshaw commented 8 years ago

I was also able to build under Cygwin (been using vis on and off under Arch VM) but I had to add -lcurses to LDFLAGS_TERMKEY. Not sure if I've got something configured wrong, or the build system.

martanne commented 8 years ago

Thanks for the feedback! I also tried it under Cygwin, problems I encountered:

Once I got it to compile it seemed to work fine (including Lua support/syntax highlighting). I noticed that the system clipboard is currently not supported, that is the "* register does not work. Search and replace :%s/foo/bar/g works if sed is installed. Apart from that vis does not have much of system integration which could fail.

joshaw commented 8 years ago

Hi Marc,

  • all configure checks fail because when given -o /dev/null the compiler attempts to create /dev/null.exe. This fails with permission denied. Maybe this is because I was running as a user without administrative rights? Did you not encounter this? This seems like a bug in the Cygwin/GCC path translation logic, /dev/null should probably receive special treatment.

https://cygwin.com/ml/cygwin/2013-09/msg00144.html

edit: better link from same thread https://cygwin.com/ml/cygwin/2013-09/msg00145.html

I didn't encounter this issue, it's possible I now have a /dev/null.exe file sitting somewhere. I do recall having this before though.

  • did you compile libtermkey from source? I could not find it in the Cygwin package repository. The configure check for libtermkey fails because it does not link to curses. This seems to be a bug in libtermkey's pkg-config file which lacks references to its dependencies. However I'm not quite sure why it works on *nix systems? Since we depend on curses anyway, we could probably just add $LDFLAGS_CURSES to the configure check of libtermkey?

Yes, I built manually. I'm actually using the fork hosted on the neovim org on github (https://github.com/neovim/libtermkey). I doubt that any proposed fixes to the lib would be merged as the dev has said a number of times that it is deprecated, but you're right the termkey.pc file is missing the dependency.

  • make local does not work
  • there were lots of warnings about redefinition of _XOPEN_SOURCE and that -fPIC would be ignored/redundant

Yep, they don't affect the build. The _XOPEN_SOURCE seems to come from CFLAGS_STD and CFLAGS_CURSESboth getting a value. And -fPIEfromCFLAGS_AUTO` causes the other messages.

  • Once I got it to compile it seemed to work fine (including Lua support/syntax highlighting). I noticed that the system clipboard is currently not supported, that is the "* register does not work. Search and replace :%s/foo/bar/g works if sed is installed. Apart from that vis does not have much of system integration which could fail.

Basic usage is good and works well, including highlighting -- have to admit to being a structural regex virgin, so not tried that branch yet. I dipped my toe in and compiled vis a number of months ago and the performance under cygwin was iffy; not a bit of that now, super speedy, impressive work, thank you.

I noticed that the lua api is not behaving though. I know that the lua part is still evolving and is not stable, but simple commands cause a crash, eg vis:command('set number') in visrc.lua cause a segmentation fault. Not sure if that's the case just for cygwin though. (vis:info('Hello world') works though.)

martanne commented 8 years ago

Regarding the /dev/null.exe issue

https://cygwin.com/ml/cygwin/2013-09/msg00145.html

suggests to use gcc -o- somefile.c >/dev/null However if my reading of the POSIX specification is correct, this is actually implementation defined behavior.

vis:command('set number') in visrc.lua causes a segmentation fault

This is likely unrelated to Cygwin. I guess you are using the above command in global scope of the visrc.lua file? It operates on the currently active window. If there is no such window it will segfault (yes this should be handled more gracefully). If you move it inside the vis.events.win_open function it should work.

martanne commented 8 years ago

With the lattest changes and assuming that ncursesw, lua-devel and lua-lpeg are installed from the Cygwin repositories the following seems to work:

make dependency/sources/install-libtermkey
PKG_CONFIG_PATH="$(pwd)/dependency/install/usr/lib/pkgconfig" ./configure
make

This is good enough for myself (I hardly ever use Windows these days).

Would be nice to get some feedback regarding *BSD support.

I would like to do a release fairly soon, such that interested people can play/hack around with vis over Easter.

erf commented 8 years ago

Building 749e72dcdae7ce8eb0c2bb47fd12aadaf4405e96 fails on OSX with output:

~/bin/vis $ ./configure && make
checking for C compiler... c99
checking whether C compiler works... yes
checking whether compiler accepts -Werror=unknown-warning-option... no
checking whether compiler accepts -Werror=unused-command-line-argument... no
checking whether linker accepts -Werror=unknown-warning-option... no
checking whether linker accepts -Werror=unused-command-line-argument... no
checking whether compiler accepts -pipe... no
checking whether compiler accepts -Os... yes
checking whether compiler accepts -fPIE... no
checking whether compiler accepts -fstack-protector-all... no
checking whether linker accepts -z now... no
checking whether linker accepts -z relro... no
checking whether linker accepts -pie... no
checking for pkg-config... yes
checking for libcurses...
 checking for ncursesw... yes
checking for libtermkey... yes
checking for liblua...
 checking for lua... yes
creating config.mk... done
c99  -Os -I/usr/local/include -I/usr/include/ncursesw   -I/usr/local/Cellar/lua/5.2.4_3/include -DLUA_COMPAT_5_1 -DLUA_COMPAT_5_2 -DLUA_COMPAT_ALL -std=c99 -D_POSIX_C_SOURCE=200809L -D_XOPEN_SOURCE=700 -DNDEBUG -D_FORTIFY_SOURCE=2 -D_DARWIN_C_SOURCE -DVERSION=\"v0.1-223-g749e72d\" -DCONFIG_LUA=1 -DCONFIG_SELINUX=0 -DCONFIG_ACL=0 *.c   -L/usr/local/lib -ltermkey -lncursesw   -L/usr/local/Cellar/lua/5.2.4_3/lib -llua -lm -lc -o vis
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c99: illegal option -- t
usage: c99 [-cEgs] [-D name[=value]] [-I directory] ... [-L directory] ...
       [-o outfile] [-O optlevel] [-U name]... [-W 64] operand ...
make: *** [vis] Error 1

I don't have much time to spend on this right now, nor am i very skilled in unix or build systems but please let me know if you want me to test something in particular.

iamleot commented 8 years ago

Dear Marc,

Marc André Tanner writes:

[...] Would be nice to get some feedback regarding *BSD support. [...] I've read your original issue comment in the last days but unfortunately I had the chance to test it only today, sorry for the delay.

I can confirm that the current "master" branch correctly works on NetBSD-current without any issues (via ``./configure'' and make(1) so using the Makefile).

Please also note that at the moment there is a vis package in pkgsrc-wip, packaged by Eric Garver:

https://wip.pkgsrc.org/cgi-bin/gitweb.cgi?p=pkgsrc-wip.git;a=tree;f=vis-editor

Keep up the good work! Ciao, L.

martanne commented 8 years ago

@erf try again, it should now work? If not try forcing the issue with make CC=clang

@iamleot no problem at all, thanks for testing and the pointer to pkgsrc-wip!

erf commented 8 years ago

@martanne it builds ok now, but when i do make install i get:

~/bin/vis-src $ make install
stripping executable
installing executable file to /bin
cp: cannot create regular file '/bin/vis': Operation not permitted
make: *** [install] Error 1

when i run ./vis in build folder it can't find default-256 unless i have set VIS_PATH, and if and if i try to open visoutside build folder the app hangs.

martanne commented 8 years ago

cp: cannot create regular file '/bin/vis': Operation not permitted

You need root privileges for installation into the system folders.

when i run ./vis in build folder it can't find default-256 unless i have set VIS_PATH

This is expected, running vis from the build directory without having set VIS_PATH is currently only supported on Linux (or more accurately on platforms where /proc/self/exe is available).

if i try to open vis outside build folder the app hangs

This should obviously not happen, please open a new issue with more detailed information. I.e. what does "hang" mean? Taking 100% CPU resources? Is just the keyboard input ignored, etc. Use a debugger to see where it hangs: gdb -p <pid-of-vis> then type bt full

I'm closing this issue now since it seems like the build works on all relevant platforms. Feel free to re-open if you encounter a problem.

Thanks to all those who provided patches and/or feedback!

mytchel commented 8 years ago

On OpenBSD configure doesn't detect lua as the pkg-config for lua-5.2 is called lua52. Adding that to the list of lua packages works fine.

malikbenkirane commented 5 years ago

On OpenBSD configure doesn't detect lua as the pkg-config for lua-5.2 is called lua52. Adding that to the list of lua packages works fine.

@martanne my current installation does not detect lua-lpeg or lua5[23]-lpeg ports this is I'm currently unable to play fairly with colors. Is there anything we need to know on OpenBSD about musl configure script ?

Should I start a new Issue ?