Skip to content

Conversation

@fingolfin
Copy link
Member

Also fix race conditions caused by targets producing files being incorrectly marked as phony.

Follow up to #59476 (guess there is a price to pay for using Claude)

@lgoettgens that might help with the issues you are seeing in JuliaPackaging/Yggdrasil#12406

Also fix race conditions caused by targets producing files
being incorrectly marked as phony.
@fingolfin fingolfin requested review from giordano and vtjnash October 28, 2025 21:33
@fingolfin fingolfin added the building Build system, or building Julia or its dependencies label Oct 28, 2025
@lgoettgens
Copy link
Contributor

Thanks for looking into this! I'll try this with the Yggdrasil script in the morning.

Could you please also add a backport-1.13 label?

Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The .PHONY is correct. Even though (or especially because) it does produce a file, it needs to be forced to re-generate it to be updated

@vtjnash
Copy link
Member

vtjnash commented Oct 28, 2025

Can you change the [email protected] files to use mktemp -p . [email protected] instead? (here and in other places where we generate files in common to release and debug, such as base/Makefile and julia_flisp.boot)

@lgoettgens
Copy link
Contributor

@lgoettgens that might help with the issues you are seeing in JuliaPackaging/Yggdrasil#12406

with this patch added, the issue happens a lot less often, but I still experience it.

@fingolfin
Copy link
Member Author

@vtjnash ah, I see. OK, calling a target $(BUILDDIR)/compile_commands.json that produces a file $(BUILDDIR)/compile_commands.json but is actually meant to be phony is an antipattern. But OK, we can just merge them into their compile-database "parent" targets. And add a comment as to why they are

I note that on my MacBook Pro M1 Max, the build system takes a full 2 seconds to rebuild these compile databases which I don't even use :-/.

A better fix would be to make it non-PHONY by properly tracking the dependencies; as far as as I can tell, that means tracking the content of CLANG_TOOLING_C_FLAGS, CLANG_TOOLING_CXX_FLAGS, SRCS and a couple other Make variables. But that'll take longer so I'll try a simpler solution.

@lgoettgens for you the much simpler fix should be to apply this patch:

diff --git a/src/Makefile b/src/Makefile
index 18e9a4a639..f92cd73a10 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -259,7 +259,7 @@ endif
 default: $(JULIA_BUILD_MODE) # contains either "debug" or "release"
 all: debug release
 
-release debug: %: libjulia-internal-% libjulia-codegen-% $(BUILDDIR)/compile_commands.json
+release debug: %: libjulia-internal-% libjulia-codegen-%
 
 $(BUILDDIR):
 	mkdir -p $(BUILDDIR)
diff --git a/src/flisp/Makefile b/src/flisp/Makefile
index 135859eacb..98b336fb14 100644
--- a/src/flisp/Makefile
+++ b/src/flisp/Makefile
@@ -61,9 +61,9 @@ DEBUGFLAGS_COMMON += $(FLAGS_COMMON)
 
 default: release
 
-release: $(BUILDDIR)/$(EXENAME)$(EXE) $(BUILDDIR)/compile_commands.json
+release: $(BUILDDIR)/$(EXENAME)$(EXE)
 
-debug: $(BUILDDIR)/$(EXENAME)-debug$(EXE) $(BUILDDIR)/compile_commands.json
+debug: $(BUILDDIR)/$(EXENAME)-debug$(EXE)
 
 $(BUILDDIR):
 	mkdir -p $(BUILDDIR)
diff --git a/src/support/Makefile b/src/support/Makefile
index 2b48a20bbc..3c1dba17f8 100644
--- a/src/support/Makefile
+++ b/src/support/Makefile
@@ -53,8 +53,8 @@ $(BUILDDIR)/host/Makefile:
 	@printf "%s\n" 'BUILDING_HOST_TOOLS=1' >> $@
 	@printf "%s\n" 'include $(SRCDIR)/Makefile' >> $@
 
-release: $(BUILDDIR)/libsupport.a $(BUILDDIR)/compile_commands.json
-debug: $(BUILDDIR)/libsupport-debug.a $(BUILDDIR)/compile_commands.json
+release: $(BUILDDIR)/libsupport.a
+debug: $(BUILDDIR)/libsupport-debug.a
 
 $(BUILDDIR)/libsupport.a: $(OBJS) | $(BUILDIR)
 	rm -rf $@

@fingolfin fingolfin requested a review from vtjnash October 31, 2025 14:02
@fingolfin
Copy link
Member Author

updated

@lgoettgens
Copy link
Contributor

Could someone please add a backport 1.13 label?

@fingolfin fingolfin merged commit ff602d4 into master Nov 1, 2025
6 of 8 checks passed
@fingolfin fingolfin deleted the mh/compile_commands branch November 1, 2025 12:08
@vtjnash
Copy link
Member

vtjnash commented Nov 4, 2025

It looks like this caused a subset of our CI workers to break:

mktemp: illegal option -- p
2025-11-03 13:51:53 EST
usage: mktemp [-d] [-q] [-t prefix] [-u] template ...
2025-11-03 13:51:53 EST
       mktemp [-d] [-q] [-u] -t prefix
2025-11-03 13:51:53 EST
/bin/sh: $TMPFILE: ambiguous redirect
2025-11-03 13:51:53 EST
usage: cmp [-l | -s | -x] [-bhz] [-i num1[:num2] | --ignore-initial=num1[:num2]] [-n num | --bytes=num] file1 file2 [skip1 [skip2]]
2025-11-03 13:51:53 EST
usage: mv [-f | -i | -n] [-hv] source target
2025-11-03 13:51:53 EST
       mv [-f | -i | -n] [-v] source ... directory
2025-11-03 13:51:53 EST
make[2]: *** [regenerate-compile_commands] Error 64

https://buildkite.com/julialang/julia-master/builds/51883#019a4b0f-2de2-40f8-9622-bf5187edcfb6

It appears you need to use mktemp $(BUILDDIR)/template.XXXX as the -p option was only added this year

KristofferC pushed a commit that referenced this pull request Nov 5, 2025
Also fix race conditions caused by targets producing files being
incorrectly marked as phony.

Follow up to  #59476 (guess there is a price to pay for using Claude)

@lgoettgens that might help with the issues you are seeing in
JuliaPackaging/Yggdrasil#12406

(cherry picked from commit ff602d4)
@KristofferC KristofferC mentioned this pull request Nov 5, 2025
17 tasks
@fingolfin
Copy link
Member Author

I've been using -p because you suggested it...

Anyway, not sure what you mean by "the -p option was only added this year" ? Added where?

The mktemp command in OpenBSD had -p since 2001, and the other BSDs followed (e.g. FreeBSD had it since at least 2005, see here). I think macOS got it not so long after, but it's a bit difficult for me to find this online; but here is at least evidence it was in macOS 12.

Not sure when GNU added it, but this man page from 2010 shows it was around then, too

So perhaps it is missing somewhere, but I am not sure what the "added last year" amounts to.

Interestingly, in the buildkite log you linked, searching for mktemp, the first hit is actually in line 14 where it does

$ mktemp -dp /private/var/tmp/agent-tempdirs/default-honeycrisp-P2C257CXGN.0/home/.buildkite-agent/plugins

without any error. (Note that -d takes no argument so -dp should be shorthand for -d -p -- but perhaps it also just ignores the p ?!)

Unfortunately I have no idea how the Buildkite build environments look like. E.h. is this build aarch64-apple-darwin builder running on macOS (which version) or is it cross compiling from Linux? Which version of tools are installed?

@vtjnash

This comment was marked as resolved.

@vtjnash
Copy link
Member

vtjnash commented Nov 7, 2025

fingolfin added a commit that referenced this pull request Nov 10, 2025
It is not supported in macOS 13 userland which causes CI failures. For
background info [see
here](#59980 (comment)).

This is a subset of PR #60080 but unlike that, it only solves the
specific issue causing trouble in CI right now, and does not attempt to
resolve a bunch of other things -- which may very well all be fine and
nice and desirable, but it's quite a bit more to review.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport 1.13 building Build system, or building Julia or its dependencies

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants