feat: use a driver script for codefile appbundles (#53)

Author: mortenpi

CHANGELOG.md

@@ -2,6 +2,12 @@
 
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## Unreleased
+
+### Changed
+
+* When submitting an appbundle with the two-argument `JuliaHub.appbundle(bundle_directory, codefile)` method, JuliaHub.jl now ensures that `@__DIR__` `@__FILE`, and `include()` in the user code now work correctly. There is a subtle behavior change due to this, where now the user script _must_ be present within the uploaded appbundle tarball (previously it was possible to use a file that would get filtered out by `.juliabundleignore`). (#37, #53)
+
 ## Version v0.1.9 - 2024-03-13
 
 ### Fixed

docs/src/guides/jobs.md

@@ -79,7 +79,8 @@ The Julia environment in the directory is also immediately added into the bundle
 
 An appbundle can be constructed with the [`appbundle`](@ref) function, which takes as arguments the path to the directory to be bundled up, and a script _within that directory_.
 This is meant to be used for project directories where you have your Julia environment in the top level of the directory or repository.
-For example, you can submit an bundle from a submit script on the top level of your project directory as follows:
+
+For example, suppose you have a script at the top level of your project directory, then you can submit a bundle as follows:
 
 ```@example
 import JuliaHub # hide
@@ -98,8 +99,9 @@ When the job starts on JuliaHub, this environment is instantiated.
 A key feature of the appbundle is that development dependencies of the environment (i.e. packages added with `pkg> develop` or `Pkg.develop()`) are also bundled up into the archive that gets submitted to JuliaHub (including any current, uncommitted changes).
 Registered packages are installed via the package manager via the standard environment instantiation, and their source code is not included in the bundle directly.
 
-When the JuliaHub job starts, the bundle is unpacked into the `appbundle/` directory (relative to the starting working directory).
-E.g. if you have a `mydata.dat` file in the bundled directory, you can access it in the script at `joinpath("appbundle", "mydata.dat")`.
+When the JuliaHub job starts, the working directory is set to the root of the unpacked appbundle directory.
+This should be kept in mind especially when launching a script that is not at the root itself, and trying to open other files from the appbundle in that script (e.g. with `open`).
+You can still use `@__DIR__` to load files relative to the script, and `include`s also work as expected (i.e. relative to the script file).
 
 Finally, a `.juliabundleignore` file can be used to exclude certain directories, by adding the relevant [globs](https://en.wikipedia.org/wiki/Glob_(programming)), similar to how `.gitignore` files work.
 In addition, `.git` directories are also automatically excluded from the bundle.

src/appbundle-driver.jl

@@ -0,0 +1,13 @@
+# This in an automatically generated driver script generated by JuliaHub.jl
+# when submitting an appbundle.
+let
+    path_components = [{PATH_COMPONENTS}]
+    path = abspath(pwd(), path_components...)
+    if !isfile(path)
+        path_relative = joinpath(path_components...)
+        error("""
+        Unable to load requested script: $(path_relative)
+         at $(path)""")
+    end
+    path
+end |> include

src/jobsubmission.jl

@@ -698,16 +698,18 @@ end
 Construct an appbundle-type JuliaHub batch job configuration. An appbundle is a directory containing a Julia environment
 that is bundled up, uploaded to JuliaHub, and then unpacked and instantiated as the job starts.
 
+The primary, two-argument method will submit a job that runs a file from within the appbundle (specified by `codefile`,
+which must be a path relative to the root of the appbundle).
 The code that gets executed is read from `codefile`, which should be a path to Julia source file relative to `directory`.
 
 ```julia
 JuliaHub.appbundle(@__DIR__, "my-script.jl")
 ```
 
-Alternatively, if `codefile` is omitted, the code can be provided as a string via the `code` keyword argument.
+Alternatively, if `codefile` is omitted, the code must be provided as a string via the `code` keyword argument.
 
 ```julia
-JuliaHub.AppBundle(
+JuliaHub.appbundle(
     @__DIR__,
     code = \"""
     @show ENV
@@ -734,17 +736,31 @@ The following should be kept in mind about how appbundles are handled:
   instantiation, and their source code is not included in the bundle directly.
 
 * When the JuliaHub job starts, the bundle is unpacked and the job's starting working directory
-  is set to the `appbundle/` directory, and you can e.g. load the data from those files with
-  just `read("my-data.txt", String)`.
-
-  Note that `@__DIR__` points elsewhere and, relatedly, `include` in the main script should be
-  used with an absolute path (e.g. `include(joinpath(pwd(), "my-julia-file.jl"))`).
+  is set to the root of the unpacked appbundle directory, and you can e.g. load the data from those
+  files with just `read("my-data.txt", String)`.
 
   !!! compat "JuliaHub 6.2 and older"
 
-      On some older JuliaHub versions (6.2 and older), the working directory was set to the parent
-      directory of `appbundle/`, and so it was necessary to do `joinpath("appbundle", "mydata.dat")`
-      to load the code.
+      On some JuliaHub versions (6.2 and older), the working directory was set to the parent directory
+      of unpacked appbundle (with the appbundle directory called `appbundle`), and so it was necessary
+      to do `joinpath("appbundle", "mydata.dat")` to load files.
+
+* When submitting appbundles with the two-argument `codefile` method, you can expect `@__DIR__` and
+  `include` to work as expected.
+
+  However, when submitting the Julia code as a string (via the `code` keyword argument), the behavior of
+  `@__DIR__` and `include` should be considered undefined and subject to change in the future.
+
+* The one-argument + `code` keyword argument method is a lower-level method, that more closely mirrors
+  the underlying platform API. The custom code that is passed via `code` is sometimes referred to as the
+  "driver script", and the two-argument method is implemented by submitting an automatically
+  constructed driver script that actually loads the specified file.
+
+!!! compat "Deprecation: v0.1.10"
+
+    As of JuliaHub.jl v0.1.10, the ability to launch appbundles using the two-argument method where
+    the `codefile` parameter point to a file outside of the appbundle itself, is deprecated. You can still
+    submit the contents of the script as the driver script via the `code` keyword argument.
 """
 function appbundle end
 
@@ -769,14 +785,56 @@ function appbundle(
 end
 
 function appbundle(bundle_directory::AbstractString, codefile::AbstractString; kwargs...)
-    codefile = abspath(joinpath(bundle_directory, codefile))
     haskey(kwargs, :code) &&
         throw(ArgumentError("'code' keyword not supported if 'codefile' passed"))
-    isfile(codefile) ||
-        throw(ArgumentError("'codefile' does not point to an existing file: $codefile"))
-    appbundle(bundle_directory; kwargs..., code=read(codefile, String))
+    codefile_fullpath = abspath(bundle_directory, codefile)
+    isfile(codefile_fullpath) ||
+        throw(ArgumentError("'codefile' does not point to an existing file: $codefile_fullpath"))
+    codefile_relpath = relpath(codefile_fullpath, bundle_directory)
+    # It is possible that the user passes a `codefile` path that is outside of the appbundle
+    # directory. This used to work back when `codefile` was just read() and submitted as the
+    # code argument. So we still support this, but print a loud deprecation warning.
+    if startswith(codefile_relpath, "..")
+        @warn """
+        Deprecated: codefile outside of the appbundle $(codefile_relpath)
+        The support for codefiles outside of the appbundle will be removed in a future version.
+        Also note that in this mode, the behaviour of @__DIR__, @__FILE__, and include() with
+        a relative path are undefined.
+
+        To avoid the warning, but retain the old behavior, you can explicitly pass the code
+        keyword argument instead of `codefile`:
+
+        JuliaHub.appbundle(
+            bundle_directory;
+            code = read(joinpath(bundle_directory, codefile), String),
+            kwargs...
+        )
+        """
+        appbundle(bundle_directory; kwargs..., code=read(codefile_fullpath, String))
+    else
+        # TODO: we could check that codefile actually exists within the appbundle tarball
+        # (e.g. to also catch if it is accidentally .juliabundleignored). This would require
+        # Tar.list-ing the bundled tarball, and checking that the file is in there.
+        driver_script = replace(
+            _APPBUNDLE_DRIVER_TEMPLATE,
+            "{PATH_COMPONENTS}" => _tuple_encode_path_components(codefile_relpath),
+        )
+        appbundle(bundle_directory; kwargs..., code=driver_script)
+    end
 end
 
+# We'll hard-code the file path directly into the driver script as string literals.
+# We trust here that repr() will take care of any necessary escaping of the path
+# components. In the end, we'll write the path "x/y/z" into the file as
+#
+#   "x", "y", "z"
+#
+# Note: splitting up the path into components also helps avoid any cross-platform
+# path separator issues.
+_tuple_encode_path_components(path) = join(repr.(splitpath(path)), ",")
+
+const _APPBUNDLE_DRIVER_TEMPLATE = read(abspath(@__DIR__, "appbundle-driver.jl"), String)
+
 function _upload_appbundle(appbundle_tar_path::AbstractString; auth::Authentication)
     isfile(appbundle_tar_path) ||
         throw(ArgumentError("Appbundle file missing: $(appbundle_tar_path)"))

test/jobenvs/job1/my-dependent-script.jl

@@ -0,0 +1 @@
+const MY_DEPENDENT_SCRIPT_1 = true

test/jobenvs/job1/script.jl

@@ -5,15 +5,31 @@ toml = TOML.parsefile(projecttoml)
 datastructures_version = toml["version"]
 
 # Check for the appbundle file
-datafile = joinpath(@__DIR__, "appbundle", "datafile.txt")
+datafile = joinpath(@__DIR__, "datafile.txt")
 datafile_hash = if isfile(datafile)
     bytes2hex(open(SHA.sha1, datafile))
 end
 
+# Try to load dependencies with relative paths:
+script_include_success = try
+    include("my-dependent-script.jl")
+    include("subdir/my-dependent-script-2.jl")
+    true
+catch e
+    e isa SystemError || rethrow()
+    @error "Unable to load"
+    false
+end
+
 results = Dict(
     "datastructures_version" => datastructures_version,
     "datafile_hash" => datafile_hash,
     "iswindows" => Sys.iswindows(),
+    "scripts" => Dict(
+        "include_success" => script_include_success,
+        "script_1" => isdefined(Main, :MY_DEPENDENT_SCRIPT_1),
+        "script_2" => isdefined(Main, :MY_DEPENDENT_SCRIPT_2),
+    ),
 )
 
 @info "Storing RESULTS:\n$(results)"

test/jobenvs/job1/subdir/my-dependent-script-2.jl

@@ -0,0 +1 @@
+const MY_DEPENDENT_SCRIPT_2 = true

test/jobs-live.jl

@@ -305,6 +305,13 @@ end
         @test VersionNumber(results["datastructures_version"]) == v"0.17.0"
         @test haskey(results, "datafile_hash")
         @test results["datafile_hash"] == datafile_hash
+        @test haskey(results, "scripts")
+        let s = results["scripts"]
+            @test s isa AbstractDict
+            @test get(s, "include_success", nothing) === true
+            @test get(s, "script_1", nothing) === true
+            @test get(s, "script_2", nothing) === true
+        end
     end
 end
 

test/jobs.jl

@@ -81,12 +81,38 @@ end
     end
 end
 
+function is_valid_julia_code(code::AbstractString)
+    try
+        ex = Meta.parse(code)
+        if ex.head === :incomplete
+            @error "Incomplete Julia expression in Julia code\n$(code)" ex
+            return false
+        end
+    catch exception
+        if isa(exception, Meta.ParseError)
+            @error "Invalid Julia code\n$(code)" exception
+            return false
+        end
+    end
+    return true
+end
+
 @testset "JuliaHub.appbundle" begin
+    driver_file_first_line = first(eachline(IOBuffer(JuliaHub._APPBUNDLE_DRIVER_TEMPLATE)))
     jobfile(path...) = joinpath(JOBENVS, "job1", path...)
 
     bundle = JuliaHub.appbundle(jobfile(), "script.jl")
     @test isfile(bundle.environment.tarball_path)
-    @test bundle.code == read(jobfile("script.jl"), String)
+    @test startswith(bundle.code, driver_file_first_line)
+    @test contains(bundle.code, "\"script.jl\"")
+    @test is_valid_julia_code(bundle.code)
+
+    bundle = JuliaHub.appbundle(jobfile(), "subdir/my-dependent-script-2.jl")
+    @test isfile(bundle.environment.tarball_path)
+    @test startswith(bundle.code, driver_file_first_line)
+    @test contains(bundle.code, "\"subdir\"")
+    @test contains(bundle.code, "\"my-dependent-script-2.jl\"")
+    @test is_valid_julia_code(bundle.code)
 
     bundle = JuliaHub.appbundle(jobfile(); code="test()")
     @test isfile(bundle.environment.tarball_path)
@@ -116,7 +142,17 @@ end
     cd(jobfile()) do
         bundle = JuliaHub.appbundle(".", "script.jl")
         @test isfile(bundle.environment.tarball_path)
-        @test bundle.code == read(jobfile("script.jl"), String)
+        @test startswith(bundle.code, driver_file_first_line)
+        @test contains(bundle.code, "\"script.jl\"")
+        @test is_valid_julia_code(bundle.code)
+    end
+
+    # Deprecated case, where `codefile` comes from outside of the appbundle
+    # directory. In that case, `codefile` gets attached directly as the driver
+    # script.
+    let bundle = @test_logs (:warn,) JuliaHub.appbundle(jobfile(), "../job-dist/script.jl")
+        @test isfile(bundle.environment.tarball_path)
+        @test bundle.code == read(jobfile("../job-dist/script.jl"), String)
     end
 end