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

normalize deps Makefile targets and add uninstall #17859

Merged
merged 6 commits into from
Sep 6, 2016

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Aug 6, 2016

using a consistent mapping between target names reduces the coupling between targets
and should make it easier to add features like reinstall, uninstall, etc.

and it fixes #17671 and #17282

while the diff is large, this essentially just a mechanical change.

all targets (except get) now use secondary file targets, giving a simple mapping from the PHONY targets, and allows another target to reference the manifest file directly rather than using the OBJ_TARGET variable:
extract: srccache/LIB/source-extracted
configure: bulid/LIB/build-configured
compile: build/LIB/build-compiled
check: build/LIB/build-checked
install: usr/manifest/LIB

this allows more accurate tracking of when a build stage has completed, and a predictable way for the user to force a build stage to restart (by removing or touching the corresponding file)

(also killed of the atlas target, since it hasn't been maintained in several years)

## phony targets ##

.PHONY: default compile install cleanall distcleanall \
get-* configure-* compile-* check-* install-* \
Copy link
Contributor

@tkelman tkelman Aug 6, 2016

Choose a reason for hiding this comment

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

extract and extract-* should probably be here?

@tkelman
Copy link
Contributor

tkelman commented Aug 6, 2016

You know what would do all of this boilerplate for us? ExternalProject_Add.

@vtjnash
Copy link
Member Author

vtjnash commented Aug 6, 2016

should still delete the shared library

it's not really that useful to delete one random file from the install, that's more a vestige of the old Makefile. in part 2 of this PR series, I'll update my PR that does a full uninstall (like autotools make uninstall, but functional even for handwritten makefiles and cmake)

@tkelman
Copy link
Contributor

tkelman commented Aug 6, 2016

very misleading to have clean-foo not actually clean anything from the installed copy of foo. otherwise you're leaving in place the piece that julia needs, so after clean-foo you'll have an old copy of the shared library sitting around and you won't notice that fact in julia tests

@tkelman
Copy link
Contributor

tkelman commented Aug 6, 2016

how about get a handwritten start on uninstall with at least the existing removals of the shared libraries

@vtjnash vtjnash force-pushed the jn/dep-makefile-normalize branch from b7800ce to be58804 Compare August 6, 2016 06:27
@vtjnash vtjnash force-pushed the jn/dep-makefile-normalize branch from 02b3c92 to c6b1d08 Compare September 6, 2016 04:39
patchelf bin is not required and was a no-op
@vtjnash vtjnash force-pushed the jn/dep-makefile-normalize branch from c6b1d08 to 2de03b0 Compare September 6, 2016 04:40
@vtjnash
Copy link
Member Author

vtjnash commented Sep 6, 2016

Anyone opposed to merging this right now, while the queues are quiet?

@tkelman
Copy link
Contributor

tkelman commented Sep 6, 2016

This is going to make everyone rebuild everything, the bugs it fixes are relatively minor.

I've also yet to see it pass on CI.

@vtjnash
Copy link
Member Author

vtjnash commented Sep 6, 2016

fwiw, I expect the x86_64 CI linalg/arnoldi here will fail, since I corrupted the arpack cache on my previous push. I can reset the cache, but then the PR build will take longer, and the cache shouldn't transfer to master after merging, does it?

@tkelman it fixes my out-of-tree build from rebuilding several things quite frequently, letting me use out-of-tree builds more heavily without the current conflicts.

@tkelman
Copy link
Contributor

tkelman commented Sep 6, 2016

Has this PR ever successfully finished a 32 bit build within the time limit?

@@ -112,9 +128,6 @@ script:
/tmp/julia/bin/julia --check-bounds=yes runtests.jl $TESTSTORUN &&
/tmp/julia/bin/julia --check-bounds=yes runtests.jl libgit2-online pkg
- cd `dirname $TRAVIS_BUILD_DIR` && mv julia2 julia &&
rm -rf julia/deps/build/julia-env &&
rm -rf julia/deps/build/libssh2-*/CMakeFiles/Makefile2 &&
rm -rf julia/deps/build/curl-*/config.log &&
Copy link
Contributor

Choose a reason for hiding this comment

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

are you positive these won't keep recaching?

Copy link
Member Author

Choose a reason for hiding this comment

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

they shouldn't, since we should no longer be touching & re-running the rules that were causing this (install should be getting handled by usr-staging.tgz now and completely ignoring deps/scratch)

and confirmed by CI: https://travis-ci.org/JuliaLang/julia/jobs/157787509#L1631

@vtjnash
Copy link
Member Author

vtjnash commented Sep 6, 2016

The 32-bit build wouldn't finish on master either if it was held to the same time limit (https://travis-ci.org/JuliaLang/julia/jobs/153542507)

@tkelman
Copy link
Contributor

tkelman commented Sep 6, 2016

The from-scratch build has gone through in under 2 hours before to repopulate a clean cache, and if it doesn't on this branch then it may be making the build slower by adding more steps. Need to find a way to fix that otherwise every build will fail.

@vtjnash
Copy link
Member Author

vtjnash commented Sep 6, 2016

ok, I don't know how to search for the last full cache build

@vtjnash
Copy link
Member Author

vtjnash commented Sep 6, 2016

anyways, full deps build for x86 have been taking ~65min on this PR. with typical travis variance, that gives about a 20% chance of failure given that the cache build times range from 45-80 minutes. and only need one build to pass to populate the cache for a long time after.

this also implements support for doing our own cache. E.g. Our buildbots can upload their usr-staging folders and travis can use those instead, eliminating that issue and the difficulties we've had with resetting corrupted caches. Plus we can enable that for everyone, so that everyone downloading the julia src repo can get lightning fast builds.

@vtjnash vtjnash merged commit 60ec350 into master Sep 6, 2016
@tkelman tkelman deleted the jn/dep-makefile-normalize branch September 6, 2016 06:21
@tkelman
Copy link
Contributor

tkelman commented Sep 6, 2016

You're neglecting the fact that compilers and glibc versions need to match to make that actually work.

@tkelman
Copy link
Contributor

tkelman commented Sep 6, 2016

Did you test this in clean compiles on all platforms?

/usr/bin/tar: ./bin: Cannot open: File exists
/usr/bin/tar: Exiting with failure status due to previous errors
make[1]: *** [/home/Tony/julia32/deps/dsfmt.mk:44: /home/Tony/julia32/usr/manifest/dsfmt] Error 2

@tkelman
Copy link
Contributor

tkelman commented Sep 6, 2016

make distcleanall reconfigures and recompiles libuv, openlibm, openspecfun, llvm and probably others. This really wasn't ready to be merged, which is why when you asked "Anyone opposed to merging this right now" I responded with objections.

echo 1 > $@

define OPENBLAS_INSTALL
$(call SHLIBFILE_INSTALL,$1,$2,$3)
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be a tab, just for visual consistency?

@vchuravy
Copy link
Member

vchuravy commented Sep 6, 2016

I am compiling with llmv 3.9 and it seems that the files are not getting install in the right place:

tar tvf /home/wallnuss/src/julia/usr-staging/llvm-3.9.0/build_Release+Asserts.tgz
drwxr-xr-x wallnuss/wallnuss 0 2016-09-07 01:22 ./

I am still looking into what causes the issue, but it seems the build is ignoring the ($build_prefix) in https://github.com/JuliaLang/julia/blob/master/deps/tools/common.mk#L146


clean-osxunwind:
-rm $(build_prefix)/manifest/libuv $(BUILDDIR)/libosxunwind-$(OSXUNWIND_VER)/build-compiled
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this deleting the libuv manifest?

@vtjnash
Copy link
Member Author

vtjnash commented Sep 6, 2016

@vchuravy
Copy link
Member

vchuravy commented Sep 6, 2016

@vtjnash thanks! I got to the same fix and was just waiting for a build finishing before submitting it. This solves my problem :)

vtjnash added a commit that referenced this pull request Feb 7, 2017
also intersperse stderr info into the log
and reduce the update interval of bar

(cherry picked from commit 1104893, PR #17859)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
building Build system, or building Julia or its dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adjust LLVM_OBJ_SOURCE
4 participants