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

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

Closed
wants to merge 1 commit into from

Conversation

jeremybobbin
Copy link
Contributor

@jeremybobbin jeremybobbin commented Jun 11, 2024

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.

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, 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
Copy link
Collaborator

rnpnr commented Jun 11, 2024

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
Copy link
Contributor Author

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
Copy link
Collaborator

rnpnr commented Jun 12, 2024

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?

#1113 (comment)

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
Copy link
Contributor Author

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
Copy link
Collaborator

rnpnr commented Jun 12, 2024

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.

@rnpnr rnpnr closed this Jun 12, 2024
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.

2 participants