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

BinaryBuilder and BinaryProvider builds #75

Merged
merged 25 commits into from
Sep 10, 2018
Merged

BinaryBuilder and BinaryProvider builds #75

merged 25 commits into from
Sep 10, 2018

Conversation

juan-pablo-vielma
Copy link
Contributor

No description provided.

deps/build.jl Show resolved Hide resolved
REQUIRE Outdated
@@ -1,5 +1,4 @@
julia 0.6
BinDeps
@osx Homebrew
BinaryProvider
Copy link
Member

Choose a reason for hiding this comment

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

Should this have a lower bound of 0.3 as per the comment in deps/build.jl?

deps/build.jl Outdated
provides(Homebrew.HB, "staticfloat/juliadeps/glpk461", glpkdep, os = :Darwin)
using BinaryProvider # requires BinaryProvider 0.3.0 or later

evalfile("build_GMP.v6.1.2.jl")
Copy link
Member

Choose a reason for hiding this comment

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

evalfile is unusual, why not include?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

deps/build.jl Outdated
provides(Homebrew.HB, "staticfloat/juliadeps/glpk461", glpkdep, os = :Darwin)
using BinaryProvider # requires BinaryProvider 0.3.0 or later

evalfile("build_GMP.v6.1.2.jl")
Copy link
Member

Choose a reason for hiding this comment

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

I don't know how BB works, but why is a build file needed for GMP given that GMP is just a dependency of GLPK and isn't referenced by the julia code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this is a "binary" dependency. Basically travis cannot compile monolithic builds so we need pre-compiler libraries like GMP. However, the you need to make sure those are also installed. In this case there is no GMP "julia package" on which GLPK could be dependent to force the install of the binaries. Resolving this is part of the BB roadmap, but it will probably use Pkg3 so we need a "hack" like this for 0.6. This gets trickier for COIN-OR solvers and it could lead to multiple versions of a library being loaded, but according to JuliaGeo/GDAL.jl/pull/52 it should be ok. Definitely worth discussing before resolving. For more details see the discussions in
https://docs.google.com/document/d/1PFw82MMGv3HEth4l1lGKEU1n71zIyEZyrIh4xJli8ck/edit?usp=sharing

deps/build.jl Outdated
# Download and install binaries
install(url, tarball_hash; prefix=prefix, force=true, verbose=verbose)
end
elseif unsatisfied
Copy link
Member

Choose a reason for hiding this comment

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

This logic is a bit different from what's in Ipopt (https://github.com/JuliaOpt/Ipopt.jl/blob/a8b12c4dfe7a9d329050bbc416c7bbe2a4722358/deps/build.jl#L37). Does it allow for local installs of GLPK outside BB?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It now allows for local installs with something like ./configure --prefix=$HOME/.julia/v0.6/GLPK/deps/usr. If it finds this it will not download any of the BB tarballs (including the GMP dependency)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, so we do need the

if haskey(download_info, platform_key())
    url, tarball_hash = download_info[platform_key()]
    if unsatisfied || !isinstalled(url, tarball_hash; prefix=prefix) 

logic to make sure binaries are installed. The logic is: If there is a tarball get its info (if haskey, download_info) and check if it has been downloaded, all the binaries it should contain are there and are not older than the downloaded file (isinstalled). If the user overwrote the binaries from a tarball those remain until there is a new tarball.

deps/verreq.jl Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Sep 7, 2018

Coverage Status

Coverage remained the same at 84.28% when pulling c1ee20f on juan-pablo-vielma:bbv0.6.1 into a694b24 on JuliaOpt:master.

@codecov-io
Copy link

codecov-io commented Sep 7, 2018

Codecov Report

Merging #75 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #75   +/-   ##
=======================================
  Coverage   52.17%   52.17%           
=======================================
  Files           4        4           
  Lines        1612     1612           
=======================================
  Hits          841      841           
  Misses        771      771

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a694b24...bc507dd. Read the comment docs.

Copy link
Member

@mlubin mlubin left a comment

Choose a reason for hiding this comment

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

It now allows for local installs with something like ./configure --prefix=$HOME/.julia/v0.6/GLPK/deps/usr. If it finds this it will not download any of the BB tarballs (including the GMP dependency)

Please document this in the README.

if Compat.Sys.isapple()
using Homebrew
provides(Homebrew.HB, "staticfloat/juliadeps/glpk461", glpkdep, os = :Darwin)
using BinaryProvider # requires BinaryProvider 0.3.0 or later
Copy link
Member

Choose a reason for hiding this comment

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

Nit: What's the point for this comment given that this requirement is listed in REQUIRE?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

those were automatically generated by BB to remind me to add to REQUIRE, which I had forgotten :-)

@mlubin mlubin merged commit 02571c2 into jump-dev:master Sep 10, 2018
This was referenced Sep 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants