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

Cache dependencies during install macro #63

Merged
merged 1 commit into from
Oct 22, 2013

Conversation

ihnorton
Copy link
Member

New usage:

@ BinDeps.install [:tk => :libtk, :tcl=>:libtcl]

Writes (to deps/deps.jl), e.g.:

const libtcl = "C:\\Users\\isaiah\\.julia\\RPMmd\\deps\\usr\\x86_64-w64-mingw32\\sys-root\\mingw\\bin\\tcl85"
const libtk = "C:\\Users\\isaiah\\.julia\\RPMmd\\deps\\usr\\x86_64-w64-mingw32\\sys-root\\mingw\\bin\\tk85"

@Keno
Copy link
Contributor

Keno commented Oct 19, 2013

Almost. If the cache is stale this should automatically rerun.

@ihnorton
Copy link
Member Author

Do you mean if the library path is not found at runtime? In that case, I would prefer to error out and suggest running Pkg.build again. The check can be put in the putative Pkg macro so that there is no runtime dependency on BinDeps.

@StefanKarpinski @JeffBezanson

@Keno
Copy link
Contributor

Keno commented Oct 19, 2013

The problem with that is that if you do using FooBar again, it will
silently fail, because the module had already been loaded.

On Saturday, October 19, 2013, Isaiah wrote:

Do you mean if the library path is not found at runtime? In that case, I
would prefer to error out and suggest running Pkg.build again. The check
can be put in the putative Pkg macro so that there is no runtime dependency
on BinDeps.

@StefanKarpinski https://github.com/StefanKarpinski @JeffBezansonhttps://github.com/JeffBezanson


Reply to this email directly or view it on GitHubhttps://github.com//pull/63#issuecomment-26653100
.

@ihnorton
Copy link
Member Author

I see, but that seems like a more general problem. Maybe we ought to have a way to abort module loading.

@JeffBezanson
Copy link

Yes, backing out of loading is a more general problem: JuliaLang/julia#3648

This change is still an improvement, since it makes the common case behave well, and pushes the badness to a case where something is already wrong.

@ihnorton
Copy link
Member Author

Also, my objection to running a build automatically is that this is not what I asked the program to do.

@Keno
Copy link
Contributor

Keno commented Oct 19, 2013

It's not running a build. It's finding the library which you did ask the
program to do.
On Saturday, October 19, 2013, Isaiah wrote:

My objection to running a build automatically is that this is not what I
asked the program to do
.


Reply to this email directly or view it on GitHubhttps://github.com//pull/63#issuecomment-26654269
.

@ihnorton
Copy link
Member Author

If I order a steak and the restaurant is sold out, I expect to be told "we have no more steak". I don't expect to be clubbed over the head or drugged for several hours while someone goes and finds another cow.

I am really, really grateful for the fact that you want things to just work, but I think that this is a step too far: triggering remote XML and RPM downloads as a consequence of using FooBar seems unreasonable.

@Keno
Copy link
Contributor

Keno commented Oct 19, 2013

That's a problem with RPMmd though.

@ihnorton
Copy link
Member Author

What do you think RPMmd should do differently?

@Keno
Copy link
Contributor

Keno commented Oct 19, 2013

Not update it's xml just because we're querying for a package.

@Keno
Copy link
Contributor

Keno commented Oct 19, 2013

I also would like to have this as part of the @load_dependencies macro instead of the @install macro. We can move that macro out into a separate file if you're concerned about loading BinDeps itself, but BinDeps is rather small. All the problem we're seeing at moment is the actual finding of the libaries (and RPMmd having millions of objects in memory).

@ihnorton
Copy link
Member Author

Not update it's xml just because we're querying for a package.

...and we've come full-circle. Frankly I don't care, but you'll have to convince @vtjnash yourself.

I also would like to have this as part of the @load_dependencies macro instead of the @install macro. We can move that macro out into a separate file if you're concerned about loading BinDeps itself, but BinDeps is rather small. All the problem we're seeing at moment is the actual finding of the libaries (and RPMmd having millions of objects in memory).

I started with that, but load_dep is recursive, and moreover I don't see the point because at the end of the day this also just calls _find_library.

@JeffBezanson
Copy link

No, you should not have to load bindeps at all just to use another package
that is already fully installed. We will go with some change like the one
presented here.
On Oct 19, 2013 3:50 PM, "Isaiah" notifications@github.com wrote:

Not update it's xml just because we're querying for a package.

...and we've come full-circlehttps://github.com/JuliaPackaging/WinRPM.jl/issues/9.
Frankly I don't care, but you'll have to convince @vtjnashhttps://github.com/vtjnashyourself.

I also would like to have this as part of the @load_dependencies macro
instead of the @install https://github.com/install macro. We can move
that macro out into a separate file if you're concerned about loading
BinDeps itself, but BinDeps is rather small. All the problem we're seeing
at moment is the actual finding of the libaries (and RPMmd having millions
of objects in memory).

I started with that, but load_dep is recursive, and moreover I don't see
the point because at the end of the day this also just calls _find_library
.


Reply to this email directly or view it on GitHubhttps://github.com//pull/63#issuecomment-26657514
.

@Keno
Copy link
Contributor

Keno commented Oct 19, 2013

Fine, but I object to having a BinDeps-specific macro in Pkg.

@JeffBezanson
Copy link

One can have a standard for where/how things are installed, with BinDeps as
a tool for putting things in that state.
On Oct 19, 2013 6:20 PM, "Keno Fischer" notifications@github.com wrote:

Fine, but I object to having a BinDeps-specific macro in Pkg.


Reply to this email directly or view it on GitHubhttps://github.com//pull/63#issuecomment-26660176
.

@Keno
Copy link
Contributor

Keno commented Oct 19, 2013

Yes, but having a cache file for the search path of libraries strikes me as an implementation detail.

@JeffBezanson
Copy link

Not really; it's not much different from having both the FHS and a distro install tool that puts files in those places.

One can say things like "package X uses the BinDeps standard", which does not in any way imply loading the full BinDeps package on every use of package X.

The way it is now, a package has to both load BinDeps and use a BinDeps-specific incantation to load its libraries. That is some pretty strong coupling. This change is in fact a step towards looser coupling. The cache file is very simple, and could be written and consumed by other tools, or written by hand. I can already imagine solving install problems by editing this file, which is certainly not preferred but it's a nice escape hatch to have.

@StefanKarpinski
Copy link
Contributor

+1

@ihnorton
Copy link
Member Author

Please let me know if there is something I should do to improve this PR. It would be nice to have more reasonable load times before releasing 0.2.

@Keno
Copy link
Contributor

Keno commented Oct 21, 2013

Alright, since I seem to have been overruled on the BinDeps runtime thing, let's put some nice error messages in here (telling you to do Pkg.build on the package you wanted to load (to make sure all dependencies are built) and then restart julia). After that I will merge this, but I reserve the right to come up with a different solution.

@JeffBezanson
Copy link

I'm fine with any solution that's at least as efficient as the one here.

@ihnorton
Copy link
Member Author

Updated. Now writes (to deps.jl):

macro checked_lib(libname, path)
    (dlopen_e(path) == C_NULL) && error("Unable to load \n\n$libname ($path)\n\nPlease re-run Pkg.build(package), and restart Julia.")
    quote const $(esc(libname)) = $path end
end
@checked_lib _jl_libpangocairo "C:\\Users\\isaiah\\.julia\\RPMmd\\deps\\usr\\x86_64-w64-mingw32\\sys-root\\mingw\\bin\\libpangocairo-1.0-0"
@checked_lib _jl_libcairo "C:\\Users\\isaiah\\.julia\\RPMmd\\deps\\usr\\x86_64-w64-mingw32\\sys-root\\mingw\\bin\\libcairo-2"
@checked_lib _jl_libgobject "C:\\Users\\isaiah\\.julia\\RPMmd\\deps\\usr\\x86_64-w64-mingw32\\sys-root\\mingw\\bin\\libgobject-2.0-0"
@checked_lib _jl_libpango "C:\\Users\\isaiah\\.julia\\RPMmd\\deps\\usr\\x86_64-w64-mingw32\\sys-root\\mingw\\bin\\libpango-1.0"

Keno added a commit that referenced this pull request Oct 22, 2013
Cache dependencies during install macro
@Keno Keno merged commit b5a575a into JuliaPackaging:master Oct 22, 2013
@JeffBezanson
Copy link

Good. How do we get packages using this? Can we just replace calls to load_dependencies with include("../deps/deps.jl")?

@ihnorton
Copy link
Member Author

That, and move the lib mapping from @ BinDeps.load_dependences [ :lib => :to ...] to @ BinDeps.install [ :lib => :to ...]. I'm working on Cairo and the stuff we need for IJulia.

@JeffBezanson
Copy link

Great, thanks.

Since we typically only hear the bad news, I'll share some good news: I was on a new ubuntu machine today, wanted to plot something, and typed Pkg.add("Winston") then using Winston and everything worked perfectly :)

@StefanKarpinski
Copy link
Contributor

Amazing! Seriously, that is great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants