-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Add BB downloading for OpenBLAS #30497
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! That looks good.
Makefile
Outdated
@@ -184,7 +184,7 @@ COMPILER_SRCS += $(shell find $(JULIAHOME)/base/compiler -name \*.jl) | |||
# sort these to remove duplicates | |||
BASE_SRCS := $(sort $(shell find $(JULIAHOME)/base -name \*.jl -and -not -name sysimg.jl) \ | |||
$(shell find $(BUILDROOT)/base -name \*.jl -and -not -name sysimg.jl)) | |||
STDLIB_SRCS := $(JULIAHOME)/base/sysimg.jl $(shell find $(build_datarootdir)/julia/stdlib/$(VERSDIR)/*/src -name \*.jl) | |||
STDLIB_SRCS := $(JULIAHOME)/base/sysimg.jl $(shell find $(JULIAHOME)/stdlib/*/src -name \*.jl) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tested this with an out-of-tree build? It also seems orthogonal got this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is a question I had for Jameson that I posted as a comment on the commit it was introduced in. Hopefully when @vtjnash reads this, he can explain to me why this is wrong. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've put this in a separate PR: #30518
11aa236
to
e005a04
Compare
c136b32
to
d85cb84
Compare
deps/openblas.version
Outdated
@@ -1,2 +1,4 @@ | |||
OPENBLAS_BRANCH=v0.3.3 | |||
OPENBLAS_SHA1=fd8d1868a126bb9f12bbc43b36ee30d1ba943fbb | |||
OPENBLAS_VER=0.3.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this 0.3.0 or 0.3.3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch; I'll build v0.3.3
in Yggdrasil, update this PR, then also submit a PR to bump to v0.3.4
, since that looks like it has some important fixes.
d85cb84
to
6fa8ed8
Compare
Also make it easier to add more BB-cached versions of dependencies in the future
000d7c7
to
628f578
Compare
Alright I think this is good to go, as long as it passes green on our CI. |
628f578
to
5df6758
Compare
contrib/fixup-libgfortran.sh
Outdated
# only get something like `@rpath/libgfortran.5.dylib` when inspecting the | ||
# libraries. We can, as a last resort, ask `$FC` directly what the full | ||
# filepath for this library is, but only if we don't have a direct path to it: | ||
if [ $(dirname "$1") == "@rpath" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't ==
also a Bash thing? And what about ${FC:-gfortran}
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
${FC:-gfortran}
should be fine, according to this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, cool. As for the conditional, you could make it if [ "$(dirname "$1")" = "@rpath" ]
.
Also allow the build system to auto-guess the triplet
5df6758
to
2c0a919
Compare
configure-$(1): | ||
compile-$(1): | ||
fastcheck-$(1): | ||
check-$(1): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these targets are incorrect and/or should not be here, they should not be empty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're here to ensure that if someone uses bb-install
within an ifeq
/else
statement, you don't run into a target fastcheck-openblas undefined
error. If the target is already defined, this won't do anything.
$$(JLDOWNLOAD) $$@ $$($(2)_BB_URL) | ||
|
||
$$(BUILDDIR)/$(1)-$$($(2)_BB_NAME)/build-compiled: | $$(BUILDDIR)/$(1)-$$($(2)_BB_NAME)/$(2).$$($(2)_BB_TRIPLET).tar.gz | ||
echo 1 > $$@ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the checksum verification should happen here before the echo 1
step
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right; we haven't been verifying checksums for the BB install tarballs. Whoopsies.
* Auto-detect binarybuilder triplet * Add OpenBLAS BinaryBuilder installation scaffolding Also make it easier to add more BB-cached versions of dependencies in the future * Enable `fixup-libgfortran.sh` to directly ask `$FC` for paths * Tell Appveyor and Travis to use BinaryBuilder OpenBLAS Also allow the build system to auto-guess the triplet
@@ -198,6 +196,7 @@ if [ -n "$USEMSVC" ]; then | |||
else | |||
# Use BinaryBuilder | |||
echo 'USE_BINARYBUILDER_LLVM = 1' >> Make.user | |||
echo 'USE_BINARYBUILDER_OPENBLAS = 1' >> Make.user | |||
echo 'BINARYBUILDER_LLVM_ASSERTS = 1' >> Make.user | |||
echo 'override DEP_LIBS += llvm openlibm' >> Make.user |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blas
needs to be added here. Right now we are still pulling it from latest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean pulling it from latest
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
julia/contrib/windows/appveyor_build.sh
Line 92 in 2c0a919
$curlflags -O https://julialangnightlies-s3.julialang.org/bin/winnt/x$archsuffix/$f |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see; so this is like a hacky way of working around USE_SYSTEM_OPENBLAS
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah... we pull in dependencies from the nightly build, then pretend that they are system libraries, but for the binarybuilder we then need too add those back... Don't ask me why I just work here... ;)
* Auto-detect binarybuilder triplet * Add OpenBLAS BinaryBuilder installation scaffolding Also make it easier to add more BB-cached versions of dependencies in the future * Enable `fixup-libgfortran.sh` to directly ask `$FC` for paths * Tell Appveyor and Travis to use BinaryBuilder OpenBLAS Also allow the build system to auto-guess the triplet
* Auto-detect binarybuilder triplet * Add OpenBLAS BinaryBuilder installation scaffolding Also make it easier to add more BB-cached versions of dependencies in the future * Enable `fixup-libgfortran.sh` to directly ask `$FC` for paths * Tell Appveyor and Travis to use BinaryBuilder OpenBLAS Also allow the build system to auto-guess the triplet (cherry picked from commit 87c18d8)
Is this needed for 1.0 releases? |
I don’t think so. Let’s not backport build fixes unless someone needs them. |
Also make it easier to use BinaryBuilder tarballs for dependencies.