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

Safer loading #7

Merged
merged 2 commits into from
Mar 22, 2022
Merged

Safer loading #7

merged 2 commits into from
Mar 22, 2022

Conversation

chriselrod
Copy link
Member

@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
Copy link

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

@chriselrod
Copy link
Member Author

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
Copy link
Member Author

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
Copy link

codecov bot commented Mar 22, 2022

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
Copy link

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
Copy link
Member Author

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
Copy link

This works:

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

@chriselrod
Copy link
Member Author

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

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.

2 participants