martanne / vis

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

fix support for incremental rebuilds as set out by 436044 #1195

Closed jeremybobbin closed 2 weeks ago

jeremybobbin commented 2 weeks ago

the POSIX Makefile macro ".c.o"(reintroduced into this patch), for example, compiles text.c -> text.o, if text.o is either older, or it doesn't exist.

this was removed in patches 4ca7119 & 09ba77a, after which, if you modify any C file(except menu & diagraph), vis would not be re-compiled.

the goal of 4ca7119 was to stop the base directory from being polluted with .o & .d files. This patch polluates the base directory.

the only simple alternative to .c.o, which doesn't pollute, is the following:

obj/%.o: %.c
    ${CC} ${CFLAGS} ${CFLAGS_VIS} -c $< -o $@

this is not POSIX, however.

rnpnr commented 2 weeks ago

Hi Jeremy,

Can you please be more specific about what you are trying to fix? 436044 is not a commit here. I have done a fair amount of development since 4ca7119 and incremental rebuilds are working just fine.

jeremybobbin commented 2 weeks ago

git log 436044 gives back:

commit 4360440862bd50525cafdcdc1a4224a1616ab486
Author: Randy Palamar <palamar@ualberta.ca>
Date:   Sat Jul 29 10:21:44 2023 -0600

    build: support incremental rebuilds

If I type make and then touch vis.c, I'd expect another call to make rebuild vis.o and then vis. Instead, says "Nothing to be done..."

Here's the output for make -d after touch vis.c:

...
  Considering target file 'obj/vis.o'.
   Pruning file 'config.mk'.
   Pruning file 'obj/.tstamp'.
  Finished prerequisites of target file 'obj/vis.o'.
   Prerequisite 'config.mk' is older than target 'obj/vis.o'.
   Prerequisite 'obj/.tstamp' is older than target 'obj/vis.o'.
  No need to remake target 'obj/vis.o'.
...

POSIX Make default rule is that if something depends on a .o file, the prerequisite .c file is in the same directory. In 4ca7119, you mentioned "some users were (rightfully) annoyed by this" - I can't seem to find the issue. Is it in a different repo?

rnpnr commented 2 weeks ago

git log 436044 gives back:

My bad, I didn't actually check the repo because I assumed that github would have hotlinked properly since it did so with the other hashes.

I can't seem to find the issue. Is it in a different repo?

https://github.com/martanne/vis/pull/1113#issuecomment-1682921399

Here's the output for make -d after touch vis.c: [...]

$ make clean && make
[...]
$ make
make: Nothing to be done for 'all'.
$ touch vis.c
$ make
cc -Wall -pipe -O2 -ffunction-sections -fdata-sections -fPIE -fstack-protector-all  -DNCURSES_WIDECHAR -I/usr/include/ncursesw    -I/usr/include/lua5.3 -DLUA_COMPAT_5_1 -DLUA_COMPAT_5_2 -DLUA_COMPAT_5_3 -DLUA_COMPAT_ALL  -std=c99 -U_XOPEN_SOURCE -D_XOPEN_SOURCE=700 -DNDEBUG -MMD -DVERSION=\"v0.9-245-gcfd6e02-dirty\" -DHAVE_MEMRCHR=1 -DVIS_PATH=\"/usr/local/share/vis\" -DCONFIG_HELP=1 -DCONFIG_CURSES=1 -DCONFIG_LUA=1 -DCONFIG_LPEG=0 -DCONFIG_TRE=0 -DCONFIG_SELINUX=0 -DCONFIG_ACL=0  -o obj/vis.o -c vis.c
cc -o vis obj/array.o obj/buffer.o obj/event-basic.o obj/libutf.o obj/main.o obj/map.o obj/sam.o obj/text-common.o obj/text-io.o obj/text-iterator.o obj/text-motions.o obj/text-objects.o obj/text-util.o obj/text.o obj/ui-terminal.o obj/view.o obj/vis-lua.o obj/vis-marks.o obj/vis-modes.o obj/vis-motions.o obj/vis-operators.o obj/vis-prompt.o obj/vis-registers.o obj/vis-subprocess.o obj/vis-text-objects.o obj/vis.o obj/text-regex.o -Wl,-z,now -Wl,-z,relro -Wl,--gc-sections -ltermkey -lncursesw -ltinfow    -llua5.3  -lc

Seems to be working fine here. If you are just saying its not POSIX I'm not particularly worried about that since its a feature which is only useful for development anyways. As long as the Makefile works with GNU make and a BSD make its good enough in my view.

For posterity which make are you using?

jeremybobbin commented 2 weeks ago

I was using GNU Make 4.4.1

If you are just saying its not POSIX I'm not particularly worried about that since its a feature which is only useful for development anyways

Which feature are you talking about? Incremental rebuilds? or .o files in the obj direcory? Regardless, I guess I was unaware of the apparent shift of values of the vis maintainership.

rnpnr commented 2 weeks ago

Regardless, I guess I was unaware of the apparent shift of values of the vis maintainership.

Shift of values? There is a time and place for POSIX pedantry and this is not one of them. We test that the project builds on Ubuntu, Debian, Alpine, OpenBSD, FreeBSD, and macOS. The fact that obj/%.o is not POSIX compatible does not affect the build on any of those systems. There are real issues to fix in vis; bikeshedding about POSIX when it does not cause any compatibility issues and when the proposed fix makes development more annoying is a waste of time.