-
-
Notifications
You must be signed in to change notification settings - Fork 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
julia-0.5.2 #3503
julia-0.5.2 #3503
Conversation
Hi! This is the friendly automated conda-forge-linting service. I wanted to let you know that I linted all conda-recipes in your PR ( Here's what I've got... For recipes/julia:
|
Hi! This is the friendly automated conda-forge-linting service. I wanted to let you know that I linted all conda-recipes in your PR ( Here's what I've got... For recipes/julia:
|
Hi! This is the friendly automated conda-forge-linting service. I wanted to let you know that I linted all conda-recipes in your PR ( Here's what I've got... For recipes/julia:
|
Hi! This is the friendly automated conda-forge-linting service. I wanted to let you know that I linted all conda-recipes in your PR ( Here's what I've got... For recipes/julia:
|
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your 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.
@dfornika thanks a bunch. Small review inline.
recipes/julia/meta.yaml
Outdated
- libgcc # [linux] | ||
- libgit2 0.24.* | ||
- gmp 0.5.* | ||
- 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.
can you pin openblas to openblas 0.2.19|0.2.19.*
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 think I've got gmp
wrong too. Will use gmp 6.1.*
recipes/julia/meta.yaml
Outdated
- libgit2 0.24.* | ||
|
||
run: | ||
- zlib |
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.
can you pin zlib to zlib 1.2.8
both run and build environments.
recipes/julia/meta.yaml
Outdated
- mpfr | ||
- curl | ||
- libssh2 1.7.* | ||
- libxml2 |
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.
please pin this to libxml2 2.9.*
A complete list can be found here: https://github.com/conda-forge/conda-forge.github.io/blob/0661e2ecf4b213ddf36350ca100a5a820eb17eb7/scripts/pin_the_slow_way.py#L40-L85
recipes/julia/meta.yaml
Outdated
- llvmdev 3.9.* | ||
- fftw | ||
- gmp 0.5.* | ||
- mpfr |
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.
mpfr 3.1.*
recipes/julia/meta.yaml
Outdated
- rpath.patch | ||
|
||
build: | ||
skip: True # [osx] |
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.
This needs to be combined in one like [py3k or osx]
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.
Skip windows builds too? (ie:)
build:
skip: True # [py3k or osx or win]
The AppVeyor CI build fails with message:
Packages missing in current win-32 channels:
- autoconf 2.69 pl5.*
- automake 1.15 pl5.*
- openblas
- gmp 0.5.*
...and
Packages missing in current win-64 channels:
- autoconf 2.69 pl5.*
- automake 1.15 pl5.*
- openblas
- gmp 0.5.*
(I realized the gmp version is wrong anyways, but not sure about the others)
recipes/julia/build.sh
Outdated
export LIBRARY_PATH=${PREFIX}/lib | ||
export CMAKE_PREFIX_PATH=${PREFIX} | ||
|
||
make -j 2 prefix=${PREFIX} MARCH=core2 sysconfigdir=${PREFIX}/etc NO_GIT=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.
Is there something like make test
which we can run?
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.
There is a test
target in the Makefile
. Can you suggest how to include that in the build script? Would we just run the make test
command first then make install
after, with the same env variables and flags? (ie:)
make -j 2 prefix=${PREFIX} MARCH=core2 sysconfigdir=${PREFIX}/etc NO_GIT=1 \
USE_SYSTEM_LIBGIT2=1 USE_LLVM_SHLIB=0 USE_SYSTEM_CURL=1 USE_SYSTEM_MPFR=1 \
USE_SYSTEM_PATCHELF=1 USE_SYSTEM_LIBSSH2=1 USE_SYSTEM_LLVM=1 USE_SYSTEM_BLAS=1 \
USE_SYSTEM_FFTW=1 USE_SYSTEM_GMP=1 USE_SYSTEM_LAPACK=1 LIBBLAS=-lopenblas \
LIBBLASNAME=libopenblas.so LIBLAPACK=-lopenblas LIBLAPACKNAME=libopenblas.so \
test
make -j 2 prefix=${PREFIX} MARCH=core2 sysconfigdir=${PREFIX}/etc NO_GIT=1 \
USE_SYSTEM_LIBGIT2=1 USE_LLVM_SHLIB=0 USE_SYSTEM_CURL=1 USE_SYSTEM_MPFR=1 \
USE_SYSTEM_PATCHELF=1 USE_SYSTEM_LIBSSH2=1 USE_SYSTEM_LLVM=1 USE_SYSTEM_BLAS=1 \
USE_SYSTEM_FFTW=1 USE_SYSTEM_GMP=1 USE_SYSTEM_LAPACK=1 LIBBLAS=-lopenblas \
LIBBLASNAME=libopenblas.so LIBLAPACK=-lopenblas LIBLAPACKNAME=libopenblas.so \
install
recipes/julia/meta.yaml
Outdated
|
||
test: | ||
commands: | ||
- "julia -v" |
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 quotes can be removed
recipes/julia/meta.yaml
Outdated
|
||
extra: | ||
recipe-maintainers: | ||
- dfornika |
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.
Feel free to add me.
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, I'm going to add @acaprez too.
CircleCI build failed with error below. Same error as seen here: The Julia github page says that libgit2 >= 0.23 should be compatible, and we're pulling libgit2 = 0.24.5 from conda-forge, so I'm not sure what the problem is.
|
It looks like the issue is that the
Running
So the first test for |
@acaprez Thanks for the insight, I would have had trouble tracking that down. Would you recommend setting Or would it be better to use the conda |
I don't actually contribute to conda-forge, so I'm unsure of what their guidelines are. For bioconda at least, I think the goal is to explicitly list and use conda packages for all the dependencies. You might try removing the version pinning for |
recipes/julia/meta.yaml
Outdated
- autoconf 2.69 pl5.* | ||
- automake 1.15 pl5.* | ||
- openblas 0.2.19|0.2.19.* | ||
- llvmdev 3.9.* |
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.
llvm 3.9 with julia 0.5.x is not a recommended configuration that would be supported in upstream bug reports
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 version would be recommended?
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.
llvm 3.7.1 (heavily patched) with julia 0.5.x, or llvm 3.9.1 (also heavily patched) with julia 0.6.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 there any news on when these patches might move upstream?
We can include patches for llvm
. There is a mechanism for it, but I haven't kept up with the latest details. Maybe @SylvainCorlay can comment.
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.
We upstream everything we can. See all the "Remove for" comments here https://github.com/JuliaLang/julia/blob/4c5cc04156ba074a8baa028c2a8a41b9e70d56ee/deps/llvm.mk#L432-L482 that mean things were successfully upstreamed. The problem is LLVM breaks other things before their next release and we haven't tested or ifdef-ed code sufficiently to work with anything else. We also haven't tested backporting our patches against other LLVM-using projects, they may cause issues in use cases that Julia doesn't care about. Realistically LLVM does not work well as a shared dependency.
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.
Can I get some advice on how to solve this? Should I remove llvm
and llvmdev
from meta.yaml
? My understanding is that the julia build system would pull down its own copy if we don't list it as a dependency and set USE_SYSTEM_LLVM=0
in the build.sh
script.
recipes/julia/meta.yaml
Outdated
|
||
test: | ||
commands: | ||
- julia -v |
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.
please run all of the tests. julia -e 'Base.runtests("all"); Base.runtests("pkg libgit2-online")'
(the ones that need network access aren't included under "all" by default, and if you build julia 0.6.x then "download" should be added to that list)
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.
Certainly a good idea to run more tests, but could you please provide us an idea of the runtime for the full test suite? Asking partially as we have to be practical if this goes over the CI time limits.
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.
Depends on the CI service. 20 minutes to an hour probably. You don't necessarily need to run them all on every minor commit, but do at least run them locally.
I'd also recommend setting TAGGED_RELEASE_BANNER
to indicate that this build of Julia comes from conda-forge, as if it fails any tests the first response from upstream will likely be that it was built in a non-recommended configuration. Passing all the tests is the best way to be confident of the build configuration, especially when it differs a lot from a normal upstream source build.
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.
Oh I missed that the tests are actually running in build.sh, that's encouraging.
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.
Should the test be run from both the build.sh
script and from the meta.yaml
file, or just one of those?
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.
Running in either place is fine. Though picking one is good. There is a slight preference to running the tests in the test
section. However if that isn't practical, running them in build.sh
is fine.
recipes/julia/rpath.patch
Outdated
# Overwrite JL_SYSTEM_IMAGE_PATH in julia library | ||
- $(call stringreplace,$(DESTDIR)$(libdir)/libjulia.$(SHLIB_EXT),sys.$(SHLIB_EXT)$$,$(private_libdir_rel)/sys.$(SHLIB_EXT)) | ||
- $(call stringreplace,$(DESTDIR)$(libdir)/libjulia-debug.$(SHLIB_EXT),sys-debug.$(SHLIB_EXT)$$,$(private_libdir_rel)/sys-debug.$(SHLIB_EXT)) | ||
+ #$(call stringreplace,$(DESTDIR)$(libdir)/libjulia.$(SHLIB_EXT),sys.$(SHLIB_EXT)$$,$(private_libdir_rel)/sys.$(SHLIB_EXT)) |
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.
why are you commenting this out? I would expect this to break things
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 don't know much about the purpose of that patch. It was written by @acaprez for the bioconda package:
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.
This may make it impossible for Julia to run except via the wrapper script.
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 don't think that's true, because the julia-0.5.2 package on bioconda uses that patch but doesn't use a wrapper script.
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.
Then is this condition even true in your build? If not, you don't need to be patching it at all.
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 don't know, but I removed the patch and re-built on my system and it seemed to work fine. It passed the test suite. I created a new conda env based on that build and ran julia, installed a simple package ("ArgParse") and imported it. Everything seemed to work.
recipes/julia/meta.yaml
Outdated
- zlib 1.2.8 | ||
- python 2.7.* | ||
- autoconf 2.69 pl5.* | ||
- automake 1.15 pl5.* |
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 were you needing autotools for? likewise with libxml2, don't usually expect that to be a build-dep
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 didn't write the original meta.yaml
file, so all I can say is that they were there in the bioconda recipe.
recipes/julia/meta.yaml
Outdated
- gmp 6.1.* | ||
- mpfr 3.1.* | ||
- curl | ||
- libssh2 1.7.* |
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.
if libgit2 depends on this at run time, then I suspect Julia's Pkg would too for handling ssh remotes (though it'll only do so through libgit2 and depend how libgit2 was built, we don't link or call libssh2 directly)
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.
So are you suggesting to add libssh2
as a runtime requirement?
It looks like the pkg
tests pass.
* pkg INFO: Initializing package repository /tmp/Zpu2ogMF/v0.5
INFO: Cloning METADATA from https://github.com/JuliaLang/METADATA.jl
INFO: No packages to install, update or remove
INFO: Cloning cache of Example from notarealprotocol://github.com/JuliaLang/Example.jl.git
INFO: Cloning cache of Example from https://github.com/JuliaLang/Example.jl.git
INFO: Installing Example v0.4.1
INFO: Package database updated
INFO: Checking out Example master...
INFO: Pulling Example latest master...
INFO: No packages to install, update or remove
INFO: Freeing Example
INFO: No packages to install, update or remove
INFO: Checking out Example master...
INFO: Pulling Example latest master...
INFO: No packages to install, update or remove
INFO: Freeing Example
INFO: No packages to install, update or remove
INFO: Removing Example v0.4.1
INFO: Package database updated
INFO: Nothing to be done
INFO: Cloning Example from https://github.com/JuliaLang/Example.jl.git
INFO: Computing changes...
INFO: No packages to install, update or remove
INFO: Package database updated
INFO: Freeing Example
INFO: No packages to install, update or remove
INFO: Checking out Example master...
INFO: Pulling Example latest master...
INFO: No packages to install, update or remove
INFO: Freeing Example
INFO: No packages to install, update or remove
INFO: Cloning Example2 from /tmp/Zpu2ogMF/v0.5/Example
INFO: Computing changes...
INFO: No packages to install, update or remove
INFO: Cloning Example3 from /tmp/Zpu2ogMF/v0.5/Example
INFO: Computing changes...
INFO: No packages to install, update or remove
INFO: Checking out Example2 test-branch-1...
INFO: Pulling Example2 latest test-branch-1...
INFO: No packages to install, update or remove
INFO: Checking out Example3 test-branch-1...
INFO: Pulling Example3 latest test-branch-1...
INFO: No packages to install, update or remove
INFO: Checking out Example master...
INFO: Pulling Example latest master...
INFO: No packages to install, update or remove
INFO: Cloning Example4 from /tmp/Zpu2ogMF/v0.5/Example
INFO: Computing changes...
INFO: No packages to install, update or remove
INFO: Checking out Example4 test-branch-2...
INFO: Pulling Example4 latest test-branch-2...
INFO: No packages to install, update or remove
INFO: Updating METADATA...
INFO: Updating Example2 test-branch-1...
INFO: Updating Example4 test-branch-2...
INFO: Updating Example3 test-branch-1...
INFO: Computing changes...
INFO: No packages to install, update or remove
INFO: Removing Example v0.4.1
INFO: Package database updated
INFO: Freeing Example
INFO: Upgrading ColorTypes: v0.2.12 => v0.5.2
INFO: Upgrading Colors: v0.6.4 => v0.7.4
INFO: Upgrading FixedPointNumbers: v0.2.1 => v0.3.9
INFO: Removing Example v0.4.1
INFO: Package database updated
WARNING: FixedPointNumbers is fixed at 0.0.6+ conflicting with requirement for ColorTypes: [0.3.0,∞)
WARNING: FixedPointNumbers is fixed at 0.0.6+ conflicting with requirement for Colors: [0.3.0,∞)
WARNING: ColorTypes is fixed at 0.0.0- conflicting with requirement for Colors: [0.3.0,∞)
WARNING: Compat is fixed at 0.2.12+ conflicting with requirement for FixedPointNumbers: [0.17.0,∞)
WARNING: Compat is fixed at 0.2.12+ conflicting with requirement for ColorTypes: [0.26.0,∞)
WARNING: Compat is fixed at 0.2.12+ conflicting with requirement for Colors: [0.18.0,∞)
INFO: Installing Example v0.4.0
INFO: Package database updated
INFO: METADATA is out-of-date — you may not have the latest version of Example
INFO: Use `Pkg.update()` to get the latest versions of your packages
in 240.46 seconds, maxrss 434.51 MB
* libgit2-online in 0.47 seconds, maxrss 434.51 MB
SUCCESS
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'd probably leave it as an indirect dependency, if libgit2 is built against it then libgit2 will have a runtime dependency on it right? Pkg will work but with degraded functionality if libgit2 happens to get built without libssh2 support. I think there are tests that would fail if that happened.
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 can remove libssh2 as it is only needed as a indirect dependency of libgit2
Green checkmark! |
How can I skip both the |
- gmp 6.1.* | ||
- mpfr 3.1.* | ||
- openblas 0.2.19|0.2.19.* | ||
- blas 1.1 {{ variant }} |
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 probably need curl
here
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.
libcurl is also indirect at runtime (for proxies in libgit2 if it's built with that support), though curl or wget is used for downloads at build and test time
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.
can keep it for the Base.download
function though (edit: on non-windows platforms, since we use something else on windows)
recipes/julia/meta.yaml
Outdated
- libgit2 | ||
|
||
run: | ||
- zlib 1.2.8 |
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.
@tkelman, is zlib
used? Or is it used by a dependency?
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.
indirect I believe
You can't. Only appveyor has a CI specific message. Others can use |
Also issue ( travis-ci/travis-ci#5032 ) is relevant. |
Guess conda doesn't have a pcre2 package yet? If that were available and had shared libraries then it would make sense to use conda's there too (build time and run time). |
No, there's not. Only |
recipes/julia/meta.yaml
Outdated
@@ -52,7 +52,8 @@ requirements: | |||
|
|||
test: | |||
commands: | |||
- julia -e 'Base.runtests("all"); Base.runtests("pkg libgit2-online")' | |||
- julia -e 'Base.runtests("all"); Base.runtests("pkg libgit2-online")': | |||
timeout: 1800s |
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.
How does this work?
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.
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.
That's CircleCI.yml functionality. This file is for conda-build
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.
That's CircleCI.yml functionality. This file is for conda-build
Suppose replacing the wrapper script with a juliarc modification can be done in the feedstock |
Thanks. Can you open 2 issues for the missing dependencies and the wrapper script in the julia-feedstocks repo once it is created? |
Looks like you have been maintaining a similar build of |
Fixes #1618