Fix & test relocatability (#3490)

* fix relocatability

* refactor shader source handling

* add relocatability test workflow

* rename workflow

* fix path

* add Test library

* fix for linux

.github/workflows/relocatability.yml

@@ -0,0 +1,45 @@
+name: Relocatability
+on:
+  pull_request:
+    paths-ignore:
+      - 'docs/**'
+      - '*.md'
+    branches:
+      - master
+  push:
+    tags:
+      - '*'
+    branches:
+      - master
+
+concurrency:
+  group: ${{ github.workflow }}-${{ github.head_ref || github.run_id }}
+  cancel-in-progress: true
+
+jobs:
+  glmakie:
+    name: GLMakie relocatability
+    env:
+      MODERNGL_DEBUGGING: "true" # turn on errors when running OpenGL tests
+    runs-on: ${{ matrix.os }}
+    strategy:
+      fail-fast: false
+      matrix:
+        version:
+          - '1' # automatically expands to the latest stable 1.x release of Julia
+        os:
+          - ubuntu-20.04
+        arch:
+          - x64
+    steps:
+      - name: Checkout
+        uses: actions/checkout@v4
+      - uses: julia-actions/setup-julia@v1
+        with:
+          version: ${{ matrix.version }}
+          arch: ${{ matrix.arch }}
+      - uses: julia-actions/cache@v1
+      - run: sudo apt-get update && sudo apt-get install -y xorg-dev mesa-utils xvfb libgl1 freeglut3-dev libxrandr-dev libxinerama-dev libxcursor-dev libxi-dev libxext-dev xsettingsd x11-xserver-utils
+      - name: Relocatability test
+        run: >
+          DISPLAY=:0 xvfb-run -s '-screen 0 1024x768x24' julia ./relocatability.jl

GLMakie/src/GLAbstraction/GLAbstraction.jl

@@ -6,6 +6,7 @@ using Makie
 using FixedPointNumbers
 using ColorTypes
 using ..GLMakie.GLFW
+using ..GLMakie: ShaderSource
 using Printf
 using LinearAlgebra
 using Observables

GLMakie/src/GLAbstraction/GLShader.jl

@@ -103,73 +103,57 @@ function ShaderCache(context)
     )
 end
 
-
 abstract type AbstractLazyShader end
+
 struct LazyShader <: AbstractLazyShader
     shader_cache::ShaderCache
-    paths::Tuple
+    paths::Vector{ShaderSource}
     kw_args::Dict{Symbol, Any}
-    function LazyShader(cache::ShaderCache, paths...; kw_args...)
+    function LazyShader(cache::ShaderCache, paths::ShaderSource...; kw_args...)
         args = Dict{Symbol, Any}(kw_args)
         get!(args, :view, Dict{String, String}())
-        new(cache, paths, args)
+        new(cache, [paths...], args)
     end
 end
 
 gl_convert(shader::GLProgram, data) = shader
 
-
-# TODO remove this silly constructor
-function compile_shader(source::Vector{UInt8}, typ, name)
-    shaderid = createshader(typ)
-    glShaderSource(shaderid, source)
+function compile_shader(source::ShaderSource, template_src::String)
+    name = source.name
+    shaderid = createshader(source.typ)
+    glShaderSource(shaderid, template_src)
     glCompileShader(shaderid)
     if !GLAbstraction.iscompiled(shaderid)
         GLAbstraction.print_with_lines(String(source))
         @warn("shader $(name) didn't compile. \n$(GLAbstraction.getinfolog(shaderid))")
     end
-    return Shader(name, source, typ, shaderid)
+    return Shader(name, Vector{UInt8}(template_src), source.typ, shaderid)
 end
 
-function compile_shader(path, source_str::AbstractString)
-    typ = shadertype(splitext(path)[2])
-    source = Vector{UInt8}(source_str)
-    name = Symbol(path)
-    return compile_shader(source, typ, name)
-end
 
-function get_shader!(cache::ShaderCache, path, template_replacement)
+function get_shader!(cache::ShaderCache, src::ShaderSource, template_replacement)
     # this should always be in here, since we already have the template keys
-    shader_dict = cache.shader_cache[path]
+    shader_dict = cache.shader_cache[src.name]
     return get!(shader_dict, template_replacement) do
-        template_source = read(path, String)
-        source = mustache_replace(template_replacement, template_source)
+        templated_source = mustache_replace(template_replacement, src.source)
         ShaderAbstractions.switch_context!(cache.context)
-        return compile_shader(path, source)
+        return compile_shader(src, templated_source)
     end::Shader
 end
 
-function get_template!(cache::ShaderCache, path, view, attributes)
-    return get!(cache.template_cache, path) do
-        _, ext = splitext(path)
-
-        typ = shadertype(ext)
-        template_source = read(path, String)
-        source, replacements = template2source(
-            template_source, view, attributes
-        )
-        s = compile_shader(path, source)
+function get_template!(cache::ShaderCache, src::ShaderSource, view, attributes)
+    return get!(cache.template_cache, src.name) do
+        templated_source, replacements = template2source(src.source, view, attributes)
+        shader = compile_shader(src, templated_source)
         template_keys = collect(keys(replacements))
         template_replacements = collect(values(replacements))
         # can't yet be in here, since we didn't even have template keys
-        cache.shader_cache[path] = Dict(template_replacements => s)
-
+        cache.shader_cache[src.name] = Dict(template_replacements => shader)
         return template_keys
     end
 end
 
-
-function compile_program(shaders, fragdatalocation)
+function compile_program(shaders::Vector{Shader}, fragdatalocation)
     # Remove old shaders
     program = createprogram()
     #attach new ones
@@ -210,55 +194,33 @@ gl_convert(lazyshader::AbstractLazyShader, data) = error("gl_convert shader")
 function gl_convert(lazyshader::LazyShader, data)
     gl_convert(lazyshader.shader_cache, lazyshader, data)
 end
+
 function gl_convert(cache::ShaderCache, lazyshader::AbstractLazyShader, data)
     kw_dict = lazyshader.kw_args
     paths = lazyshader.paths
-    if all(x-> isa(x, Shader), paths)
-        fragdatalocation = get(kw_dict, :fragdatalocation, Tuple{Int, String}[])
-        ShaderAbstractions.switch_context!(cache.context)
-        return compile_program([paths...], fragdatalocation)
-    end
+
     v = get_view(kw_dict)
     fragdatalocation = get(kw_dict, :fragdatalocation, Tuple{Int, String}[])
 
-    # Tuple(Source, ShaderType)
-    if all(paths) do x
-            isa(x, Tuple) && length(x) == 2 &&
-            isa(first(x), AbstractString) &&
-            isa(last(x), GLenum)
-        end
-        # we don't cache view & templates for shader strings!
-        shaders = map(paths) do source_typ
-            source, typ = source_typ
-            src, _ = template2source(source, v, data)
-            ShaderAbstractions.switch_context!(cache.context)
-            compile_shader(Vector{UInt8}(src), typ, :from_string)
-        end
-        ShaderAbstractions.switch_context!(cache.context)
-        return compile_program([shaders...], fragdatalocation)
-    end
-    if !all(x -> isa(x, AbstractString), paths)
-        error("Please supply only paths or tuples of (source, typ) for Lazy Shader
-            Found: $paths"
-        )
-    end
     template_keys = Vector{Vector{String}}(undef, length(paths))
     replacements = Vector{Vector{String}}(undef, length(paths))
-    for (i, path) in enumerate(paths)
-        template = get_template!(cache, path, v, data)
+
+    for (i, shader_source) in enumerate(paths)
+        template = get_template!(cache, shader_source, v, data)
         template_keys[i] = template
         replacements[i] = String[mustache2replacement(t, v, data) for t in template]
     end
+
     return get!(cache.program_cache, (paths, replacements)) do
         # when we're here, this means there were uncached shaders, meaning we definitely have
         # to compile a new program
         shaders = Vector{Shader}(undef, length(paths))
-        for (i, path) in enumerate(paths)
+        for (i, shader_source) in enumerate(paths)
             tr = Dict(zip(template_keys[i], replacements[i]))
-            shaders[i] = get_shader!(cache, path, tr)
+            shaders[i] = get_shader!(cache, shader_source, tr)
         end
         ShaderAbstractions.switch_context!(cache.context)
-        compile_program(shaders, fragdatalocation)
+        return compile_program(shaders, fragdatalocation)
     end
 end
 
@@ -346,7 +308,6 @@ function mustache2replacement(mustache_key, view, attributes)
 end
 
 # Takes a shader template and renders the template and returns shader source
-template2source(source::Vector{UInt8}, view, attributes::Dict{Symbol, Any}) = template2source(String(source), attributes, view)
 function template2source(source::AbstractString, view, attributes::Dict{Symbol, Any})
     replacements = Dict{String, String}()
     source = mustache_replace(source) do mustache_key

GLMakie/src/GLAbstraction/GLTypes.jl

@@ -28,7 +28,7 @@ struct Shader
     id::GLuint
     context::GLContext
     function Shader(name, source, typ, id)
-        new(name, source, typ, id, current_context())
+        new(Symbol(name), source, typ, id, current_context())
     end
 end
 

GLMakie/src/GLMakie.jl

@@ -41,16 +41,29 @@ end
 import ShaderAbstractions: Sampler, Buffer
 export Sampler, Buffer
 
+struct ShaderSource
+    typ::GLenum
+    source::String
+    name::String
+end
+
+function ShaderSource(path)
+    typ = GLAbstraction.shadertype(splitext(path)[2])
+    source = read(path, String)
+    name = String(path)
+    return ShaderSource(typ, source, name)
+end
+
 const GL_ASSET_DIR = RelocatableFolders.@path joinpath(@__DIR__, "..", "assets")
 const SHADER_DIR = RelocatableFolders.@path joinpath(GL_ASSET_DIR, "shader")
-const LOADED_SHADERS = Dict{String, String}()
+const LOADED_SHADERS = Dict{String, ShaderSource}()
 
 function loadshader(name)
     # Turns out, joinpath is so slow, that it actually makes sense
     # To memoize it :-O
     # when creating 1000 plots with the PlotSpec API, timing drop from 1.5s to 1s just from this change:
     return get!(LOADED_SHADERS, name) do
-        return joinpath(SHADER_DIR, name)
+        return ShaderSource(joinpath(SHADER_DIR, name))
     end
 end
 

GLMakie/test/unit_tests.jl

@@ -6,6 +6,46 @@ function project_sp(scene, point)
     return point_px .+ offset
 end
 
+@testset "shader cache" begin
+    GLMakie.closeall()
+    screen = display(Figure())
+    cache = screen.shader_cache
+    # Postprocessing shaders
+    @test length(cache.shader_cache) == 5
+    @test length(cache.template_cache) == 5
+    @test length(cache.program_cache) == 4
+
+    # Shaders for scatter + linesegments + poly etc (axis)
+    display(screen, scatter(1:4))
+    @test length(cache.shader_cache) == 16
+    @test length(cache.template_cache) == 16
+    @test length(cache.program_cache) == 10
+
+    # No new shaders should be added:
+    display(screen, scatter(1:4))
+    @test length(cache.shader_cache) == 16
+    @test length(cache.template_cache) == 16
+    @test length(cache.program_cache) == 10
+
+    # Same for linesegments
+    display(screen, linesegments(1:4))
+    @test length(cache.shader_cache) == 16
+    @test length(cache.template_cache) == 16
+    @test length(cache.program_cache) == 10
+
+    # Lines hasn't been compiled so one new program should be added
+    display(screen, lines(1:4))
+    @test length(cache.shader_cache) == 18
+    @test length(cache.template_cache) == 18
+    @test length(cache.program_cache) == 11
+
+    # For second time no new shaders should be added
+    display(screen, lines(1:4))
+    @test length(cache.shader_cache) == 18
+    @test length(cache.template_cache) == 18
+    @test length(cache.program_cache) == 11
+end
+
 @testset "unit tests" begin
     GLMakie.closeall()
     @testset "Window handling" begin
@@ -250,7 +290,7 @@ end
 
         @test screen.root_scene === nothing
         @test screen.rendertask === nothing
-        @test (Base.summarysize(screen) / 10^6) < 1.22
+        @test (Base.summarysize(screen) / 10^6) < 1.4
     end
     # All should go to pool after close
     @test all(x-> x in GLMakie.SCREEN_REUSE_POOL, screens)

relocatability.jl

@@ -0,0 +1,50 @@
+
+module_src = """
+module MakieApp
+
+using GLMakie
+
+function julia_main()::Cint
+    screen = display(scatter(1:4))
+    # wait(screen) commented out to test if this blocks anything, but didn't change anything
+    return 0 # if things finished successfully
+end
+
+end # module MakieApp
+"""
+
+using Pkg, Test
+
+makie_dir = pwd()
+tmpdir = mktempdir()
+# create a temporary project
+cd(tmpdir)
+Pkg.generate("MakieApp")
+Pkg.activate("MakieApp")
+
+paths = [makie_dir, joinpath(makie_dir, "MakieCore"), joinpath(makie_dir, "GLMakie")]
+
+Pkg.develop(map(x-> (;path=x), paths))
+
+open("MakieApp/src/MakieApp.jl", "w") do io
+    print(io, module_src)
+end
+
+Pkg.activate(".")
+Pkg.add("PackageCompiler")
+
+using PackageCompiler
+
+create_app(joinpath(pwd(), "MakieApp"), "executable"; force=true, incremental=true, include_transitive_dependencies=false)
+exe = joinpath(pwd(), "executable", "bin", "MakieApp")
+@test success(`$(exe)`)
+julia_pkg_dir = joinpath(Base.DEPOT_PATH[1], "packages")
+@test isdir(julia_pkg_dir)
+mvd_julia_pkg_dir = julia_pkg_dir * ".old"
+# Move package dir so that we can test relocatability (hardcoded paths to package dir being invalid now)
+try
+    mv(julia_pkg_dir, mvd_julia_pkg_dir)
+    @test success(`$(exe)`)
+catch e
+    mv(mvd_julia_pkg_dir, julia_pkg_dir)
+end