Skip to content

Commit

Permalink
use magic to fix make -j operation race condition on certain targets
Browse files Browse the repository at this point in the history
  • Loading branch information
@vtjnash
vtjnash committed Feb 15, 2015
1 parent 7cafb22 commit e85a1a5
Showing 1 changed file with 26 additions and 6 deletions.
32 changes: 26 additions & 6 deletions deps/Makefile
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,27 @@ JULIAHOME = $(abspath ..)
include Versions.make
include $(JULIAHOME)/Make.inc

# Special comments:
#
# all targets in here should follow the same structure,
# and provide a get-a, configure-a, compile-a, check-a, and install-a
# additionally all targets should be listed in the getall target for easier off-line compilation
# if you are adding a new target, it can help to copy an similar, existing target

This comment has been minimized.

Sign in to view

Copy link
@StefanKarpinski

StefanKarpinski Feb 15, 2015

Member

OMG, comments!! Thank you for doing this. Arguably we needed this even back when it was just me and Viral working on this file :-P
#
# autoconf configure-driven scripts: llvm pcre arpack fftw unwind gmp mpfr patchelf uv
# custom Makefile rules: openlibm Rmath-julia dsfmt suitesparse-wrapper suitesparse lapack openblas mojibake objconv
# CMake libs: libgit2
#
# downloaded from git: llvm-svn, uv, libopenlibm, mojibake, openspecfun
#
# there are rules in this file with the . replaced by a %
# this is some magic Makefile trick that tells make
# that all targets with a % in them on that line will
# be rebuilt in a single invocation
#

## Some shared configuration options ##

CONFIGURE_COMMON = --prefix=$(abspath $(build_prefix)) --build=$(BUILD_MACHINE) --libdir=$(abspath $(build_libdir))
ifneq ($(XC_HOST),)
CONFIGURE_COMMON += --host=$(XC_HOST)
Expand Down Expand Up @@ -31,9 +52,8 @@ endif
# they will override the values passed above to ./configure
MAKE_COMMON = DESTDIR="" prefix=$(build_prefix) bindir=$(build_bindir) libdir=$(build_libdir) libexecdir=$(build_libexecdir) datarootdir=$(build_datarootdir) includedir=$(build_includedir) sysconfdir=$(build_sysconfdir)

#autoconf configure-driven scripts: llvm pcre arpack fftw unwind gmp mpfr patchelf uv
#custom Makefile rules: openlibm Rmath-julia dsfmt suitesparse-wrapper suitesparse lapack openblas mojibake objconv
#CMake libs: libgit2

## Overall configuration of which rules exist and should be run by default ##

# prevent installing libs into usr/lib64 on opensuse
unexport CONFIG_SITE
Expand Down Expand Up @@ -685,7 +705,7 @@ endif
$(OPENLIBM_OBJ_SOURCE): openlibm/Makefile
$(MAKE) -C openlibm $(OPENLIBM_FLAGS) $(MAKE_COMMON)
touch -c $@
$(OPENLIBM_OBJ_TARGET): $(OPENLIBM_OBJ_SOURCE)
$(build_shlibdir)/libopenlibm%$(SHLIB_EXT) $(build_libdir)/libopenlibm%a : $(OPENLIBM_OBJ_SOURCE)
$(MAKE) -C openlibm install $(OPENLIBM_FLAGS) $(MAKE_COMMON)
$(INSTALL_NAME_CMD)libopenlibm.$(SHLIB_EXT) $(build_shlibdir)/libopenlibm.$(SHLIB_EXT)
touch -c $@
Expand Down Expand Up @@ -748,7 +768,7 @@ install-openspecfun: $(OPENSPECFUN_OBJ_TARGET)

## DSFMT ##

DSFMT_OBJ_TARGET = $(build_shlibdir)/libdSFMT.$(SHLIB_EXT)
DSFMT_OBJ_TARGET = $(build_shlibdir)/libdSFMT.$(SHLIB_EXT) $(build_includedir)/dSFMT.h
DSFMT_OBJ_SOURCE = dsfmt-$(DSFMT_VER)/libdSFMT.$(SHLIB_EXT)

DSFMT_CFLAGS = $(CFLAGS) -DNDEBUG -DDSFMT_MEXP=19937 $(fPIC) -DDSFMT_DO_NOT_USE_OLD_NAMES
Expand All @@ -774,7 +794,7 @@ dsfmt-$(DSFMT_VER)/config.status: dsfmt-$(DSFMT_VER).tar.gz
$(DSFMT_OBJ_SOURCE): dsfmt-$(DSFMT_VER)/config.status
cd dsfmt-$(DSFMT_VER) && \
$(CC) $(CPPFLAGS) $(DSFMT_CFLAGS) $(LDFLAGS) dSFMT.c -o libdSFMT.$(SHLIB_EXT)
$(DSFMT_OBJ_TARGET): $(DSFMT_OBJ_SOURCE)
$(build_shlibdir)/libdSFMT%$(SHLIB_EXT) $(build_includedir)/dSFMT%h: $(DSFMT_OBJ_SOURCE) | $(build_includedir) $(build_shlibdir)
cp dsfmt-$(DSFMT_VER)/dSFMT.h $(build_includedir)
cp $< $@ && \
$(INSTALL_NAME_CMD)libdSFMT.$(SHLIB_EXT) $(DSFMT_OBJ_TARGET)
Expand Down

12 comments on commit e85a1a5

@StefanKarpinski
Copy link
Member

Choose a reason for hiding this comment

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

This is great, assuming it works – does it seem to? Also, do you have a reference for the % business?

@vtjnash
Copy link
Member Author

Choose a reason for hiding this comment

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

yes, i proved that this works.

it's in the manual: http:https://www.gnu.org/software/make/manual/make.html#Pattern-Intro

If a pattern rule has multiple targets, make knows that the rule’s recipe is responsible for making all of the targets. The recipe is executed only once to make all the targets.

however, i would have read that sentence a hundred times without picking up on its significance without the SO mention: http:https://stackoverflow.com/a/3077254/1712368

@tkelman
Copy link
Contributor

Choose a reason for hiding this comment

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

We are getting a lot of new appveyor timeouts since yesterday during compilation of C code which didn't usually happen before, there's a chance this change is confusing the old msys1 make that we use on appveyor.

@tkelman
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's see whether d2c1841 helps at all there, might be a slightly newer less freeze-prone version of make than what I was using

@vtjnash
Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes. Using msys1 make in parallel is known to result in corrupt, partial, and/or hung builds. (This used to be mentioned in the windows readme, but probably got removed when we stopped recommending msys1 at all)

@tkelman
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I remember, I ran into that plenty of times on projects before Julia. Little surprising that we haven't hit it on AppVeyor yet though, few if any of the timeouts before these makefile changes were ever during make.

@tkelman
Copy link
Contributor

Choose a reason for hiding this comment

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

Come to think of it I have occasionally seen make.exe segfaulting on AppVeyor, but it was pretty rare.

@vtjnash
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think I've seen a crash. Lots of hangs, and occasionally, binaries generated that crash or fail tests

@tkelman
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think I've seen a crash.

Like I said, make.exe crashing is rare.

Lots of hangs

Which almost always occur either during sysimg bootstrap or while running the first test. Never during compilation of C code until these makefile changes. The different version of make looks okay so far.

binaries generated that crash or fail tests

Which also happens locally. Not unique to appveyor. 0.4 is just buggy still.

@vtjnash
Copy link
Member Author

Choose a reason for hiding this comment

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

This change made much better use of make's parallelism. Previously, we were still forcing a bit more to happen in serial

Yes 0.4 still has bugs. But I seemed to find that parallel msys1 make would also manage to create bugs in functional code

@tkelman
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, if you were referring to MSYS1 make in general and not AppVeyor builds specifically, then nevermind.

@tkelman
Copy link
Contributor

Choose a reason for hiding this comment

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

Still freezing with a different version of make. https://ci.appveyor.com/project/StefanKarpinski/julia/build/1.0.2592/job/gr0ek33vo3yc41fw

We'll have to disable parallel make on AppVeyor because of this. Cygwin or MSYS2 make could be used but need to be downloaded and cached.

Please sign in to comment.