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

Return of the build script via Preferences #483

Merged
merged 26 commits into from
Nov 3, 2022

Conversation

mkitti
Copy link
Contributor

@mkitti mkitti commented Nov 1, 2022

This restores the build script to facilitate downloading the GR binary. It is currently imported via GRPreferences. The main purpose of restoring the build script is as a fallback when the BinaryBuilder binary does not work. For example, Wayland support is currently lacking in the BinaryBuilder QT builds.

Currently, this script can be invoked via

using GR
GR.GRPreferences.use_upstream_binary()

I'll probably change the function name to make it clearer that this downloads a binary.

cc: @t-bltg

@mkitti
Copy link
Contributor Author

mkitti commented Nov 1, 2022

Wayland issues xref: sciapp/gr#141

@codecov-commenter
Copy link

codecov-commenter commented Nov 1, 2022

Codecov Report

Base: 40.71% // Head: 38.00% // Decreases project coverage by -2.71% ⚠️

Coverage data is based on head (fbbada0) compared to base (f224e83).
Patch coverage: 1.47% of modified lines in pull request are covered.

❗ Current head fbbada0 differs from pull request most recent head 9a82ca6. Consider uploading reports for the commit 9a82ca6 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #483      +/-   ##
==========================================
- Coverage   40.71%   38.00%   -2.72%     
==========================================
  Files           7        8       +1     
  Lines        2692     2892     +200     
==========================================
+ Hits         1096     1099       +3     
- Misses       1596     1793     +197     
Impacted Files Coverage Δ
src/downloader.jl 0.00% <0.00%> (ø)
src/preferences.jl 12.62% <3.84%> (-21.87%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mkitti
Copy link
Contributor Author

mkitti commented Nov 1, 2022

cc: @sjkelly , it turns out the alternate binaries are still useful for Wayland support.

@t-bltg
Copy link
Contributor

t-bltg commented Nov 1, 2022

I think that the build script can be simplified, instead of just copy-paste what was removed in #465.

@sjkelly
Copy link
Contributor

sjkelly commented Nov 1, 2022

This sounds reasonable to me. I appreciate everyone filling in the gaps in my artifacts PR. I assume this doesn't break precompilation on 1.8+?

@mkitti
Copy link
Contributor Author

mkitti commented Nov 1, 2022

I think that the build script can be simplified, instead of just copy-paste what was removed in #465.

Yes, I'm working on it. I just wanted to develop a proof of concept to see if the old script still works before I starting cutting it down.

This sounds reasonable to me. I appreciate everyone filling in the gaps in my artifacts PR. I assume this doesn't break precompilation on 1.8+?

I have not modified the __init__ process. We're entirely using the Preferences.jl mechanism still. Basically what I'm restoring is the ability to automatically download the correct upstream GR tarball.

To decide:

  1. Name of the download function. use_deps_binary does not seem descriptive.
  2. Where to put the downloaded tarball. I'm not sure if the deps folder is the right place.
  3. We might be able to use the artifacts system still.... just specifying these as custom artifacts.

@t-bltg
Copy link
Contributor

t-bltg commented Nov 1, 2022

I assume this doesn't break precompilation on 1.8+

Xref timholy/SnoopCompile.jl#295, this would have to be tested against Plots precompilation mechanism using SnoopPrecompile, (just try using Plots with this branch dev'ed for GR).

@jheinen
Copy link
Owner

jheinen commented Nov 1, 2022

Yes, this would simplify the installation of the GR tarball for many users. However, the runtime problem under Linux still exists. I could only achieve correct results with the LD_PRELOAD environment variable (see sciapp/gr#141 (comment))

@mkitti
Copy link
Contributor Author

mkitti commented Nov 1, 2022

However, the runtime problem under Linux still exists. I could only achieve correct results with the LD_PRELOAD environment variable

Addressing #484 may resolve the issue. Basically if we use dont_dlopen=true, then the GR_jll binaries will never be loaded if Preferences points to the other binaries.

@jheinen
Copy link
Owner

jheinen commented Nov 2, 2022

@mkitti : Thanks for your PR.

Would it be possible to "control" the behaviour from outside Julia, e.g.
GRDIR=/usr/local/gr julia ...
without restarting Julia (when preferences have been changed).

The background of my question is: We use Julia in a read-only network environment with pre-installed packages (which are maintained daily) for all users. Possibly a strange requirement, but the process has proven itself.

@mkitti
Copy link
Contributor Author

mkitti commented Nov 2, 2022

Would it be possible to "control" the behaviour from outside Julia, e.g. GRDIR=/usr/local/gr julia ... without restarting Julia (when preferences have been changed).

Yes. Essentially you could preconfigure by providing an Overrides.toml in the depot and a LocalPreferences.toml in the project.

Depot based Overrides.toml

The first mechanism is older and allows for overriding the binary locations via a depot based Overrides.toml [1].

julia> using GR
[ Info: Precompiling GR [28b8d3ca-fb5f-59d9-8090-bfdbd6d07a71]

julia> GR.GRPreferences.use_upstream_binary(; force = true, override = :depot)
┌ Info: Downloading pre-compiled GR 0.69.1 Ubuntu binary
└   file = "/tmp/jl_rbiTw2/gr-0.69.1-Ubuntu-x86_64.tar.gz"
┌ Info: grdir
└   grdir = "/home/mkitti/.julia/dev/GR/deps/gr"
[ Info: Please restart Julia to change the GR binary configuration.

What that does is create an Overrides.toml in the Julia depot:

$ cat ~/.julia/artifacts/Overrides.toml
[d2c73de3-f751-5644-a686-071e5b155ba9]
GR = "/home/mkitti/.julia/dev/GR/deps/gr"

Thus, one option that you have is to simply create that Overrides.toml in the depot. The UUID there is that of GR_jll:
https://github.com/JuliaBinaryWrappers/GR_jll.jl/blob/fc933dae4a54c49707193e2dcc7df1e5ef201cab/Project.toml#L2

julia> GR_jll.libGR
"/home/mkitti/.julia/dev/GR/deps/gr/lib/libGR.so"

julia> GR_jll.libGR3
"/home/mkitti/.julia/dev/GR/deps/gr/lib/libGR3.so"

julia> GR_jll.libGRM
"/home/mkitti/.julia/dev/GR/deps/gr/lib/libGRM.so"

julia> GR_jll.gksqt_path
"/home/mkitti/.julia/dev/GR/deps/gr/bin/gksqt"

julia> DEPOT_PATH
3-element Vector{String}:
 "/home/mkitti/.julia"
 "/home/mkitti/src/julia-1.8.1/local/share/julia"
 "/home/mkitti/src/julia-1.8.1/share/julia"

Note that above not only have we instructed GR to load the alternate binaries, but we've now also redirected GR_jll to alternate binaries. This should comprehensively take care of the binary issue. You should not need LD_PRELOAD because GR_jll actually is loading the same binaries that we want. Yes, this is currently a bit redundant.

Recall also that depots stack on the system depots. You could also locate the Overrides.toml in

/home/mkitti/src/julia-1.8.1/local/share/julia/artifacts/Overrides.toml
/home/mkitti/src/julia-1.8.1/share/julia/artifacts/Overrides.toml

It also still makes the modification to the LocalPreferences.toml stored in the same directory as the Project.toml to memorize this configuration setting:

$ cat LocalPreferences.toml 
[GR]
binary = "system"
grdir = "/home/mkitti/.julia/dev/GR/deps/gr"

Project based LocalPreferences.toml configuration

Another option involves only using LocalPreferences.toml, stored in the project [2]. Here I've installed GR into /home/mkitti/gr.

julia> using GR

julia> GR.GRPreferences.use_upstream_binary("/home/mkitti"; force = true, override = :project)
┌ Info: Downloading pre-compiled GR 0.69.1 Ubuntu binary
└   file = "/tmp/jl_4IzraC/gr-0.69.1-Ubuntu-x86_64.tar.gz"
┌ Info: grdir
└   grdir = "/home/mkitti/gr"
[ Info: Please restart Julia to change the GR binary configuration.

Here the paths to the individual binaries is stored in the LocalPreferences.toml.

$ cat LocalPreferences.toml 
[GR]
binary = "system"
grdir = "/home/mkitti/gr"

[GR_jll]
gksqt_path = "/home/mkitti/gr/bin/gksqt"
libGKS_path = "/home/mkitti/gr/lib/libGKS"
libGR3_path = "/home/mkitti/gr/lib/libGR3"
libGRM_path = "/home/mkitti/gr/lib/libGRM"
libGR_path = "/home/mkitti/gr/lib/libGR"

Summary

Yes, you can control which binaries are loaded by defining either JULIA_DEPOT_PATH/artifacts/Overrides.toml or /path/to/project/LocalPreferences.toml as above. Writing the TOML files can be done completely outside Julia. Before this PR, the LocalPreferences.toml only redirected GR to use different binaries. With this PR, GR_jll is now also redirected to use alternate binaries. This in part addresses the LD_PRELOAD issue.

References

[1] Overrides.toml in the Pkg.jl documentation. https://pkgdocs.julialang.org/v1/artifacts/#Overriding-artifact-locations
[2] (LocalPreferences.toml) in the BinaryBuilder.jl documentation. https://docs.binarybuilder.org/stable/jll/#Non-dev'ed-JLL-packages

@mkitti
Copy link
Contributor Author

mkitti commented Nov 2, 2022

Would it be possible to "control" the behaviour from outside Julia, e.g.
GRDIR=/usr/local/gr julia ...

To clarify, the GRDIR environment variable override will also continue to function, but we need JuliaPackaging/Yggdrasil#5784 to avoid the need for LD_PRELOAD.

@jheinen
Copy link
Owner

jheinen commented Nov 2, 2022

@mkitti : Thanks for the detailed explanations. If I can merge, just give a quick shout.

Comment on lines +64 to +72
function get_version()
_version = version
if "GRDIR" in keys(ENV)
if isempty(ENV["GRDIR"])
_version = "latest"
end
end
return _version
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we pin GR_jll in https://github.com/jheinen/GR.jl/blob/master/Project.toml, and remove this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any thoughts about this @jheinen . Do we need the version to be latest if the GRDIR environment variable is set?

Copy link
Owner

Choose a reason for hiding this comment

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

No, for development purposes, there are probably better ways to test the latest run-time. End users don't need this.

src/downloader.jl Outdated Show resolved Hide resolved
src/downloader.jl Outdated Show resolved Hide resolved
src/downloader.jl Outdated Show resolved Hide resolved
src/downloader.jl Outdated Show resolved Hide resolved
src/preferences.jl Outdated Show resolved Hide resolved
@mkitti mkitti marked this pull request as ready for review November 3, 2022 00:10
src/downloader.jl Outdated Show resolved Hide resolved
@mkitti
Copy link
Contributor Author

mkitti commented Nov 3, 2022

The functionality is where I would like it.

However, I think we should still do the following:

  1. Add tests
  2. Add documentation

What is the best way to update the Sphinx documentation at gr-framework.org? Otherwise, I'll just update the README.

Perhaps we should implement Documenter.jl based documentation?

@jheinen
Copy link
Owner

jheinen commented Nov 3, 2022

What is the best way to update the Sphinx documentation at gr-framework.org? Otherwise, I'll just update the README.

To get ahead quickly, the README file should be updated. The official GR documentation is unfortunately only in a private repo.

Perhaps we should implement Documenter.jl based documentation?

Would probably make sense in the medium term. I would have to find out how to integrate this sensibly.

@mkitti
Copy link
Contributor Author

mkitti commented Nov 3, 2022

Would probably make sense in the medium term. I would have to find out how to integrate this sensibly.

I can help open another pull request on this after we merge this.

@mkitti
Copy link
Contributor Author

mkitti commented Nov 3, 2022

There definitely seems to be some kind of race condition going on with Volume plot

@mkitti
Copy link
Contributor Author

mkitti commented Nov 3, 2022

@jheinen Let's merge this as soon as possible since this fixes the plugin breakage as described in #486.

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.

5 participants