search

JuliaSIMD / CPUSummary.jl

MIT License
7 stars 4 forks source link

Safer loading #7

Closed chriselrod closed 2 years ago

chriselrod commented 2 years ago

@tkf @fredrikekre Is this better? Or should I take an approach like JuliaLang/julia/pull/43270? Also, I'm not sure how to test if this actually works. The current tests were passing even in the broken versions.

fredrikekre commented 2 years ago

Seems fine. The main concern should be whether Hwloc is available though.

chriselrod commented 2 years ago

Seems fine. The main concern should be whether Hwloc is available though.

Why may it not be? It's part of CPUSummary's project, so Base.lower_path_setup_code() should make it available, if the project's been instantiated?

Of course something seems to be going wrong, so it's probable that I am mistaken somewhere.

chriselrod commented 2 years ago

If running Julia on WINE, using Hwloc will crash Julia. It's important for this package to at least not crash Julia when run under WINE (or in any other situation either, really).

Hence, it's meant to fail in situations like that. But it isn't great if Hwloc would've worked fine, but it's for some reason not available in the process launched by run while it would have been under a simple using.

codecov[bot] commented 2 years ago

Codecov Report

Merging #7 (ed9b3aa) into main (a9a0204) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##             main       #7   +/-   ##
=======================================
  Coverage   38.58%   38.58%           
=======================================
  Files           3        3           
  Lines         184      184           
=======================================
  Hits           71       71           
  Misses        113      113
Impacted Files Coverage Δ
src/CPUSummary.jl 18.18% <ø> (ø)

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 a9a0204...ed9b3aa. Read the comment docs.

fredrikekre commented 2 years ago

But you are only allowed to load "top level packages", and if Hwloc hasn't been explicitly Pkg.added by the user it will fail.

chriselrod commented 2 years ago

But you are only allowed to load "top level packages", and if Hwloc hasn't been explicitly Pkg.added by the user it will fail.

Ah. CPUSummary.jl is itself allowed to using Hwloc. One should thus be allowed to using Hwloc when using the CPUSummary.jl project.

I guess Base.load_path_setup_code() creates setup code for the main project, and not from the scope/environment it is called from?

fredrikekre commented 2 years ago

This works:

julia> Hwloc = Base.require(Base.PkgId(Base.UUID("0e44f5e4-bd66-52a0-8798-143a42290a1d"), "Hwloc"))
Hwloc
chriselrod commented 2 years ago

Excellent!

julia> using Hwloc
 │ Package Hwloc not found, but a package named Hwloc is available from a registry.
 │ Install package?
 │   (p3) pkg> add Hwloc
 └ (y/n/o) [y]: n
ERROR: ArgumentError: Package Hwloc not found in current path.
- Run `import Pkg; Pkg.add("Hwloc")` to install the Hwloc package.
Stacktrace:
 [1] macro expansion
   @ ./loading.jl:1047 [inlined]
 [2] macro expansion
   @ ./lock.jl:223 [inlined]
 [3] require(into::Module, mod::Symbol)
   @ Base ./loading.jl:1028

julia> Base.require(Base.PkgId(Base.UUID("0e44f5e4-bd66-52a0-8798-143a42290a1d"), "Hwloc"))
[ Info: Precompiling Hwloc [0e44f5e4-bd66-52a0-8798-143a42290a1d]
Hwloc