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 target dependencies aren't correct #981

Closed
TurkeyMan opened this issue Dec 19, 2017 · 12 comments · Fixed by #1005
Closed

Makefile target dependencies aren't correct #981

TurkeyMan opened this issue Dec 19, 2017 · 12 comments · Fixed by #1005

Comments

@TurkeyMan
Copy link
Contributor

TurkeyMan commented Dec 19, 2017

I've noticed that the make output has lots of errors with rule dependencies, and this leads to problems with multi-core builds (ie, make -j2 or more)

Consider:

all: prebuild prelink $(TARGET) | $(TARGETDIR) $(OBJDIR)
	@:

What this says is that all depends on prebuild prelink and $(TARGET), but it misses the hierarchy of dependency.
Truth is, all depends on $(TARGET), which depends on prelink, which depends on $(OBJECTS), which may-or-may-not depend on $(PCH), which depends on prebuild.
We need to restructure the dependency resolution into the proper dependency structure, otherwise multi-threaded doesn't work.

Currently, -j3 may cause prebuild, prelink, and $(TARGET) to all be built at the same time in parallel, which is wrong, since each step may depend on successful completion of the prior steps.

@samsinsane
Copy link
Member

Does this occur with gmake2? From what I recall, one of your colleagues had some pretty good insight into the new and improved action (and Makefiles in general), and if I'm not mistaken, it resolved issues with multiple "jobs". Perhaps I was mistaken and it was only certain things that it resolved? I don't recall who it was, but I can probably dig through the PRs and find them, unless you already know who it is?

@TurkeyMan
Copy link
Contributor Author

Yes, I'm using gmake 2 ;)
No idea who it was... this is a big place, and I'm not in close contact with that team.

It's just bugs effectively.. If I get around to it, I'll make a patch, just wanted to record the issue here.
The rule dependencies are just not in the right places. For instance all depends on prebuild, but it should be $(OBJECTS) that depends on prebuild... cascade upwards.

@samsinsane
Copy link
Member

It's just bugs effectively.. If I get around to it, I'll make a patch, just wanted to record the issue here.

Fair enough. 👍 One day I should sit down and spend some time learning the ins and outs of Makefiles so I can actually give some input with these things.

No idea who it was... this is a big place, and I'm not in close contact with that team.

FWIW, it was @bwhittle - I'm guessing what you're suggesting could speed up their build times, so I guess you might have additional devs looking into this with you? 😄

@TurkeyMan
Copy link
Contributor Author

TurkeyMan commented Dec 20, 2017

Is there anyone here that can comment on this:

all: prebuild prelink $(TARGET) | $(TARGETDIR) $(OBJDIR)
	@:

$(TARGET): $(GCH) $(OBJECTS) $(LDDEPS) | $(TARGETDIR)
	@echo Linking Premake5
	$(SILENT) $(LINKCMD)
	$(POSTBUILDCMDS)

$(OBJECTS): $(GCH) $(PCH) | $(OBJDIR) $(PCH_PLACEHOLDER)
$(GCH): $(PCH) | $(OBJDIR)
	@echo $(notdir $<)
	$(SILENT) $(CC) -x c-header $(ALL_CFLAGS) -o "$@" -MF "$(@:%.gch=%.d)" -c "$<"

This is just a fragment from a premake generated makefile, but there are a few questions I have:

  1. You can see that all depends on $(TARGET) and soft-depends $(TARGETDIR), and then that $(TARGET) also depends on $(TARGETDIR)... Is there a reason all needs that dependency too? It should be implied recursively.
  2. Same story; $(OBJECTS) depends on $(GCH) and $(PCH), but then ($GCH) also depends on $(PCH). Same story as above... is there a reason it's repeated in both places?

@starkos
Copy link
Member

starkos commented Dec 21, 2017

This code has been touched by a lot of people over the years…I think your assessment is likely correct, and those dependencies ought to be untangled.

@TurkeyMan
Copy link
Contributor Author

Okay, so I feel like it should look something like this:

all: $(TARGET)
	@:

$(TARGET): prelink | $(TARGETDIR)
	@echo Linking Premake5
	$(SILENT) $(LINKCMD)
	$(POSTBUILDCMDS)

prebuild:
	$(PREBUILDCMDS)

prelink: $(OBJECTS) $(LDDEPS)
	$(PRELINKCMDS)

$(OBJECTS): prebuild $(GCH) $(PCH) | $(OBJDIR) $(PCH_PLACEHOLDER)
$(GCH): $(PCH) | $(OBJDIR)
	@echo $(notdir $<)
	$(SILENT) $(CC) -x c-header $(ALL_CFLAGS) -o "$@" -MF "$(@:%.gch=%.d)" -c "$<"

I actually think prebuild should probably be a dependency of $(GCH), but I tried that and it got weird.
Can anyone who might be better at make than me suggest any improvements?
Also, I wonder, since prebuild and prelink are just arbitrary rules, how does make determine that these are 'dirty' and rebuild them or not?

@starkos
Copy link
Member

starkos commented Dec 22, 2017

This looks reasonable to me (though my Makefile-fu is not particularly strong, especially when it comes to parallel builds).

@TurkeyMan
Copy link
Contributor Author

I'm looking at this; can anyone explain PCH_PLACEHOLDER?

@TurkeyMan
Copy link
Contributor Author

I've been working on this for the past 2 days, and the further I go, the more completely fubar I realise the existing make code is. make -j# is totally broken.
I also realise that I don't think I have the skills to fix it in a reasonable timeframe.

It all falls apart on prebuild events, and I need to conclude how they should behave...
Is the answer "like visual studio"? I'm not sure everything matches that pattern.
Visual studio does: if any thing is dirty, prebuild runs. Is nothing is dirty, prebuild doesn't run... This means that in VS, prebuild is unable to affect any source files and cause them to rebuild. Prebuild does not always run.

This situation is particularly hard to express in make, since the conditions that will cause prebuild to re-run aren't easily expressed as make dependencies.

Do we have any make experts floating around?

Here's an example:

.PHONY: all

SRCS := src/poo.x src/poo2.x src/poo3.x src/poo4.x
OBJS := $(SRCS:src/%.x=int/%.o)

all: out/poo.out

out/poo.out: $(OBJS) | out/
	@echo prelink
	touch $@
	@echo postbuild

out/:
	mkdir out
int/:
	mkdir int

$(OBJS): int/prebuild | int/
	@echo $@
	@sleep 1
	@touch $@

int/prebuild: $(SRCS)
	@echo prebuild
	@sleep 1
	touch int/prebuild

Use make -j2 to build.

Challenge for the reader: consider the prebuild rule; it depends on $(SRCS) such that any change to src files (which triggers rebuild) will cause prebuild to re-run... problem is, this code rebuilds EVERYTHING in the event only a single source file changes.

@TurkeyMan
Copy link
Contributor Author

Okay, I think I've worked out a satisfactory solution.

Can everyone tell me they agree this looks good?

.PHONY: all

SRCS := src/poo.x src/poo2.x src/poo3.x src/poo4.x
OBJS := $(SRCS:src/%.x=int/%.o)

all: out/poo.out

out/poo.out: $(OBJS) | out/
	@echo prelink
	touch $@
	@echo postbuild

out/:
	mkdir out
int/:
	mkdir int

int/poo.o: src/poo.x | int/prebuild
	@echo $@
	@sleep 1
	@touch $@
int/poo2.o: src/poo2.x | int/prebuild
	@echo $@
	@sleep 1
	@touch $@
int/poo3.o: src/poo3.x | int/prebuild
	@echo $@
	@sleep 1v
	@touch $@
int/poo4.o: src/poo4.x | int/prebuild
	@echo $@
	@sleep 1
	@touch $@

int/prebuild: $(SRCS) | int/ 
	@echo prebuild
	@sleep 1
	touch int/prebuild

@TurkeyMan
Copy link
Contributor Author

TurkeyMan commented Feb 16, 2018

Nope, I discovered a few issues...

  1. In VS, if you need to relink, but NOT rebuild any source files, prebuild does run. With my rules here; prebuild will only re-run if any source files need to rebuild...
  2. Dependencies of source files (headers) will not cause the prebuild to re-run, even though the source file will rebuild.

Are these acceptable limitations for now? It's definitely better than what premake currently emits.

@starkos
Copy link
Member

starkos commented Feb 20, 2018

I say yes, they are acceptable limitations, and someone else may be able to build out more from here.

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.

3 participants