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

Resource loading #135

Merged
merged 5 commits into from Mar 4, 2019
Merged

Resource loading #135

merged 5 commits into from Mar 4, 2019

Conversation

squaremo
Copy link
Member

@squaremo squaremo commented Feb 22, 2019

This implements a "magic" module '@jkcfg/std/resource'. It's magic
because it's generated each time it's imported, so that it can take
into account the path of the importing module (known at import time,
and not thereafter).

The implementation is this:

  • give the read procedure an option module for specifying a module to use as the base path (a missing value means use the input directory);
  • when @jkcfg/std/resource is imported, hash the current import base path (i.e., the directory of the importing module), then load module code exporting a procedure that reads using the hash.

This has some flaws:

  • at present, jk (v8worker2, really) gives imports using the same
    specifier (in this case @jkcfg/std/resource) the same module,
    which is not much use for magic modules.

  • it relies on the module base path being readable (if you change the
    input path when you run jk, it might not be)

(EDIT: strike out flaws, which have been excised, and explain how module base paths are identified.)

@squaremo squaremo force-pushed the module-relative-reads branch 3 times, most recently from d1a0f6e to 81d6116 Compare March 2, 2019 19:43
@squaremo squaremo changed the title [WIP] resource loading Resource loading Mar 2, 2019
This implements a "magic" module '@jkcfg/std/resource'. It's magic
because it's generated each time it's imported, so that it can take
into account the path of the importing module (known at import time,
and not thereafter).

The implementations is this: create a module that embeds the base path
of the module, and exports a procedure that does reads relative to the
base path.

This has some flaws:

 - at present, jk (v8worker2, really) gives imports using the same
   specifier (in this case `@jkcfg/std/resource`) the same module,
   which is not much use for magic modules.

 - it relies on the module base path being readable (if you change the
   input path when you run jk, it might not be)
This commit adds a test that resource() can succeed even if the module
directory is not under the input directory. The test does not pass,
since all reads are currently required to be under the input
directory.

It also corrects the guard against reading outside of the input
directory: it's possible for os/path/filepath.Rel to return a relative
path that starts with parent directories, i.e., with `..`.
Just using std.read() for resources won't work in general, because the
desired path may not be under the input directory (and reads are
restricted to paths under the read directory).

To enable reads from module directories, while keeping the guard
against reading outside the allowed paths, keep track of each module
that imports `@jkcfg/std/resource' and look up the module path when
its `resource` procedure is called, to use as a base path.
To be able to identify the base path of a resource read, I added a
`module` option to `read`. That is now "generally available", meaning
any read might include it, if it can guess a module hash, and read
from some other module's base path.

To prevent abuse of this mechanism, salt the module hash using random
bytes generated when starting up. The randomness does not hinder
repeatability, since the hashes are opaque and only used in the scope
of a run anyway.
@dlespiau
Copy link
Member

dlespiau commented Mar 4, 2019

One thing I'm not sure of is the resource API using import to load a specific resource. That means the std.read hidden in the magic importer cannot receive arguments to specify how the resource should be read.

I totally misread the test case :) all good :)

Copy link
Member

@dlespiau dlespiau left a comment

Choose a reason for hiding this comment

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

\o/ this looks awesome to me!

// as generating the magic modules when they are imported.
type ModuleResources struct {
// module hash -> basePath for resource reads
modules map[string]string
Copy link
Member

Choose a reason for hiding this comment

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

I think we can't manipulate this map from different goroutines, so we indeed don't need locking?

Copy link
Member Author

@squaremo squaremo Mar 4, 2019

Choose a reason for hiding this comment

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

At present it won't happen, but I could be defensive and put a mutex on it

@squaremo squaremo merged commit 802cb42 into master Mar 4, 2019
@squaremo squaremo deleted the module-relative-reads branch March 4, 2019 21:25
@dlespiau dlespiau mentioned this pull request Mar 5, 2019
@bryanlarsen bryanlarsen mentioned this pull request Apr 16, 2019
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.

None yet

2 participants