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

Problem with new version with packages that load other packages #48

Open
davidanthoff opened this issue Jul 11, 2018 · 6 comments
Open

Comments

@davidanthoff
Copy link
Contributor

Ok, now for a real problem with version v0.5.1 that has the @timholy rewrite.

I have a package that I stripped down to:

__precompile__()
module IterableTables

using Requires

function __init__()
    @info("INIT CALLED")
    @require DataFrames="a93c6f00-e57d-5684-b7b6-d8193f3e46c0" println("WORKED")    
end

end # module

When I load it, I get:

               _
   _       _ _(_)_     |  A fresh approach to technical computing
  (_)     | (_) (_)    |  Documentation: https://docs.julialang.org
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 0.7.0-beta.238 (2018-07-10 21:07 UTC)
 _/ |\__'_|_|_|\__'_|  |  Commit 5d1a3fcba1* (0 days old master)
|__/                   |  x86_64-w64-mingw32

julia> using DataFrames

julia> using IterableTables
[ Info: INIT CALLED
[ Info: Precompiling module DataFrames
ERROR: LoadError: ArgumentError: Package Reexport not found in current path:
 - Run `Pkg.add("Reexport")` to install the Reexport package.

Stacktrace:
 [1] require(::Module, ::Symbol) at .\loading.jl:816
 [2] include at .\boot.jl:317 [inlined]
 [3] include_relative(::Module, ::String) at .\loading.jl:1034
 [4] include(::Module, ::String) at .\sysimg.jl:29
 [5] top-level scope at none:0
 [6] eval at .\boot.jl:319 [inlined]
 [7] eval(::Expr) at .\client.jl:394
 [8] top-level scope at .\none:3 [inlined]
 [9] top-level scope at .\<missing>:0
in expression starting at C:\Users\david\.julia\packages\DataFrames\Pv1t\src\DataFrames.jl:10
┌ Warning: Error requiring DataFrames from IterableTables:
│ Failed to precompile DataFrames to C:\Users\david\.julia\compiled\v0.7\DataFrames.ji.
│ Stacktrace:
│  [1] error(::String) at .\error.jl:33
│  [2] macro expansion at .\logging.jl:298 [inlined]
│  [3] compilecache(::Base.PkgId) at .\loading.jl:1169
│  [4] _require(::Base.PkgId) at .\loading.jl:942
│  [5] require(::Base.PkgId) at .\loading.jl:838
│  [6] top-level scope at none:0
│  [7] eval at .\boot.jl:319 [inlined]
│  [8] eval at C:\Users\david\.julia\dev\IterableTables\src\IterableTables.jl:2 [inlined]
│  [9] (::getfield(IterableTables, Symbol("##3#6")))() at C:\Users\david\.julia\packages\Requires\uXN8\src\require.jl:67
│  [10] err(::getfield(IterableTables, Symbol("##3#6")), ::Module, ::String) at C:\Users\david\.julia\packages\Requires\uXN8\src\require.jl:38
│  [11] #2 at C:\Users\david\.julia\packages\Requires\uXN8\src\require.jl:66 [inlined]
│  [12] withpath(::getfield(IterableTables, Symbol("##2#5")), ::String) at C:\Users\david\.julia\packages\Requires\uXN8\src\require.jl:28
│  [13] #1 at C:\Users\david\.julia\packages\Requires\uXN8\src\require.jl:65 [inlined]
│  [14] listenpkg(::getfield(IterableTables, Symbol("##1#4")), ::Base.PkgId) at C:\Users\david\.julia\packages\Requires\uXN8\src\require.jl:13
│  [15] __init__() at .\logging.jl:297
│  [16] _include_from_serialized(::String, ::Array{Any,1}) at .\loading.jl:600
│  [17] macro expansion at .\logging.jl:300 [inlined]
│  [18] _require_search_from_serialized(::Base.PkgId, ::String) at .\loading.jl:682
│  [19] _require(::Base.PkgId) at .\loading.jl:919
│  [20] require(::Base.PkgId) at .\loading.jl:838
│  [21] require(::Module, ::Symbol) at .\loading.jl:833
│  [22] eval(::Module, ::Any) at .\boot.jl:319
│  [23] eval_user_input(::Any, ::REPL.REPLBackend) at C:\cygwin\home\Administrator\buildbot\worker\package_win64\build\usr\share\julia\stdlib\v0.7\REPL\src\REPL.jl:85
│  [24] macro expansion at C:\cygwin\home\Administrator\buildbot\worker\package_win64\build\usr\share\julia\stdlib\v0.7\REPL\src\REPL.jl:116 [inlined]
│  [25] (::getfield(REPL, Symbol("##28#29")){REPL.REPLBackend})() at .\task.jl:257
└ @ Requires C:\Users\david\.julia\packages\Requires\uXN8\src\require.jl:40

julia>

If I then add Reexport to my global environment, I get an error that StatsBase is not found. StatsBase is the second package that is being used from DataFrames.jl.

So I guess it looks as if a package A that is being required by package B won't properly load other packages that are in the REQUIRE file for A.

@timholy
Copy link
Member

timholy commented Jul 11, 2018

Thanks! I am short on time, so let me just say it seems to work if you comment out this line. @vtjnash predicted this and proposed an alternative solution (#46 (comment)).

@timholy
Copy link
Member

timholy commented Jul 11, 2018

In the short term, rather than a full glue package I suspect this might work:

@require DataFrames="a93c6f00-e57d-5684-b7b6-d8193f3e46c0" include("extras_dataframes.jl")

where extras_dataframes.jl lives in the same src and looks something like

using DataFrames

function mycoolfunction(df::DataFrame, ...)
    body
end

But if you wanted that code precompiled you'd definitely want the auxillary module as proposed by @vtjnash.

@jmert
Copy link
Contributor

jmert commented Jul 12, 2018

I don't understand why the original doesn't work, but the following patch seems to solve the problem in a similar usage case.

diff --git a/src/require.jl b/src/require.jl
index e402421..93391bd 100644
--- a/src/require.jl
+++ b/src/require.jl
@@ -58,14 +58,14 @@ macro require(pkg, expr)
     return Expr(:macrocall, Symbol("@warn"), __source__,
                 "Requires now needs a UUID; please see the readme for changes in 0.7.")
   id, modname = parsepkg(pkg)
-  pkg = Base.PkgId(Base.UUID(id), modname)
+  pkg = :(Base.PkgId(Base.UUID($id), $modname))
   quote
     if !isprecompiling()
-      listenpkg(Base.PkgId(Base.UUID($id), $modname)) do
+      listenpkg($pkg) do
         withpath($(string(__source__.file))) do
-          err($__module__, $(pkg.name)) do
+          err($__module__, $modname) do
             $(esc(:(eval($(Expr(:quote, Expr(:block,
-                                            :(const $(Symbol(pkg.name)) = Base.require($pkg)),
+                                            :(const $(Symbol(modname)) = Base.require($pkg)),
                                             expr)))))))
           end
         end

@vtjnash
Copy link
Member

vtjnash commented Jul 12, 2018

It would be better for compile times (and memory usage, and readability, and debugging / backtraces), if we could make this macro expand to a simple function call, rather than a dense nest of new function definitions:

macro requires(pkg, expr)
  id, modname = parsepkg(pkg)
  return :(do_requires($__module__, $(QuoteNode(__source__.file)), $(QuoteNode(id)), $(QuoteNode(modname)), $(QuoteNode(expr))))
end

function do_requires(m::Module, this_file, id, modname, expr)
    pkg = Base.PkgId(Base.UUID(id), modname))
    if !isprecompiling()
        listenpkg(pkg) do
            withpath(string(thisfile)) do
                err(m, modname) do
                    Core.eval(m, quote
                        const $(Symbol(modname)) = $(Base.require(pkg))
                        $expr
                    end)
                    nothing
                end
                nothing
           end
           nothing
        end
        nothing
    end
    nothing
end

@MikeInnes
Copy link
Collaborator

@jmert that's very odd; can you send a PR and @davidanthoff can you confirm if that patch fixes things for you?

I unfortunately can't reproduce this (on 0.7-beta) – maybe something has changed, it'd be good to verify.

@jmert
Copy link
Contributor

jmert commented Jul 12, 2018

@jmert that's very odd; can you send a PR and @davidanthoff can you confirm if that patch fixes things for you?

Will do. PR coming shortly.

MikeInnes added a commit that referenced this issue Jul 12, 2018
Interpolate PkgId expression, working around #48
@jmert jmert mentioned this issue Jul 12, 2018
MikeInnes added a commit that referenced this issue Jul 16, 2018
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

No branches or pull requests

5 participants