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

makefile.gen mistakenly thinks all my objects are up-to-date #354

Closed
joeyparrish opened this issue Sep 24, 2024 · 4 comments · Fixed by #355
Closed

makefile.gen mistakenly thinks all my objects are up-to-date #354

joeyparrish opened this issue Sep 24, 2024 · 4 comments · Fixed by #355

Comments

@joeyparrish
Copy link
Contributor

When I build, then change a source, then rebuild, nothing happens. I'm using the docker image with the :latest tag on Linux.

I went through recent changes and tested before and after 8b07ad7 ("re-introduced deps"). That is definitely the point at which the regression was introduced. If I build the docker image from the commit just before this, it all works correctly.

For a more concrete example, I have src/main.c, which doesn't get rebuilt when I change it. out/src/main.d contains:

out/src/main.o: src/main.c /sgdk/inc/genesis.h /sgdk/inc/types.h \
 /sgdk/inc/config.h /sgdk/inc/asm.h /sgdk/inc/sys.h /sgdk/inc/sram.h \
 /sgdk/inc/mapper.h /sgdk/inc/memory.h /sgdk/inc/memory_base.h \
 /sgdk/inc/tools.h /sgdk/inc/bmp.h /sgdk/inc/maths.h /sgdk/inc/vdp.h \
 /sgdk/inc/pal.h /sgdk/inc/dma.h /sgdk/inc/vdp_tile.h /sgdk/inc/vdp_bg.h \
 /sgdk/inc/map.h /sgdk/inc/pool.h /sgdk/inc/object.h /sgdk/inc/font.h \
 /sgdk/res/libres.h /sgdk/inc/string.h /sgdk/inc/tab_cnv.h \
 /sgdk/inc/maths3D.h /sgdk/inc/vdp_spr.h /sgdk/inc/vdp_pal.h \
 /sgdk/inc/vram.h /sgdk/inc/sprite_eng.h /sgdk/inc/sprite_eng_legacy.h \
 /sgdk/inc/z80_ctrl.h /sgdk/inc/ym2612.h /sgdk/inc/psg.h \
 /sgdk/inc/snd/sound.h /sgdk/inc/snd/xgm.h /sgdk/inc/snd/xgm2.h \
 /sgdk/inc/snd/smp_null.h /sgdk/inc/snd/smp_null_dpcm.h \
 /sgdk/inc/snd/pcm/snd_pcm.h /sgdk/inc/snd/pcm/snd_dpcm2.h \
 /sgdk/inc/snd/pcm/snd_pcm4.h /sgdk/inc/joy.h /sgdk/inc/timer.h \
 /sgdk/inc/task.h /sgdk/inc/task_cst.h /sgdk/inc/ext/flash-save/flash.h \
 /sgdk/inc/types.h /sgdk/inc/ext/flash-save/saveman.h \
 /sgdk/inc/ext/console.h /sgdk/inc/config.h /sgdk/inc/string.h \
 /sgdk/inc/maths.h /sgdk/inc/dma.h

I can't spot the problem. Could you please take a look? Maybe even consider reverting the change while you investigate, since the :latest docker tag is currently broken? Or giving a named tag to an older build, maybe something like :stable?

Thanks!

@Stephane-D
Copy link
Owner

Stephane-D commented Sep 26, 2024

I think this is a problem with the Docker build script as it works as expected when used natively on Windows. And actually this fixed the dependencies: before that modifying a .h file didn't rebuilt any files, now each C file including that header is properly rebuilt.
I think the issue about the docker script is just a path issue, it seems that it can find the .d files but cannot properly find the dependencies inside so it just ignore the files list.

@joeyparrish
Copy link
Contributor Author

I finally found the root cause. What I failed to notice originally was the target make said was up-to-date:

$ ./build.sh
make: 'out/res/kinetoscope_logo.o' is up to date.

That's not my ROM...

When you type "make" without specifying a target, it builds the first target in the makefile. However, the first target isn't what you'd think. This is because of this code in makefile.gen:

OBJ= $(RES_RES:.res=.o)
OBJ+= $(RES_S:.s=.o)
OBJ+= $(RES_C:.c=.o)
OBJ+= $(SRC_S80:.s80=.o)
OBJ+= $(SRC_ASM:.asm=.o)
OBJ+= $(SRC_S:.s=.o)
OBJ+= $(SRC_C:.c=.o)
OBJS:= $(addprefix $(OUT)/, $(OBJ))

DEPS:= $(OBJS:.o=.d)

-include $(DEPS)

The included deps files define targets. So the first target is the thing in the first deps file, which is based on the first object in OBJS.

The next targets are those explicitly defined in the makefile: release:, followed by debug:, followed by asm:, then $(SRC)/main.c:, then whatever might be included from ext.mk, and then finally all: and default:. all: depends on release, so "make all" still does what it should.

By convention, all: is often the first target in a makefile, but the name itself isn't special. It's the position that matters to make it the default target.

So, if you are in the habit of typing "make all", you wouldn't run into this bug.

(Incidentally, the Dockerfile entrypoint by default doesn't specify a target, but you can pass one as an argument.)

The solution is simply to move all: or default: to the top of the file.

joeyparrish added a commit to joeyparrish/SGDK that referenced this issue Sep 30, 2024
The DEPS support added in 8b07ad7 accidentally broke the default makefile target by including deps above the `all`, `default`, or `release` targets.  Previously, the default (first) target had been `release`.

This moves `all` and `default` to the top, both of which depend on `release`.

Fixes Stephane-D#354
@Stephane-D
Copy link
Owner

Oh yeah that's true, the deps file define targets, and then they become the default target. Thanks for pointing it out !
I guess i'm always typing the target so I didn't experienced that.

Stephane-D pushed a commit that referenced this issue Sep 30, 2024
The DEPS support added in 8b07ad7 accidentally broke the default makefile target by including deps above the `all`, `default`, or `release` targets.  Previously, the default (first) target had been `release`.

This moves `all` and `default` to the top, both of which depend on `release`.

Fixes #354
@joeyparrish
Copy link
Contributor Author

Happy to help! Thanks for making an amazing tool!

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 a pull request may close this issue.

2 participants