awslabs / llrt

LLRT (Low Latency Runtime) is an experimental, lightweight JavaScript runtime designed to address the growing demand for fast and efficient Serverless applications.
Apache License 2.0
7.73k stars 341 forks source link

docs: Clarify instructions for running emulator locally #391

Closed nikp closed 3 weeks ago

nikp commented 1 month ago

Description of changes

Without explicitly setting LAMBDA_TASK_ROOT, make run fails to find index by looking it one level higher than it should (ReferenceError: Error resolving module '/Users/nppinski/workspace/index' from '' -> meanwhile, my working directory is workspace/llrt) due to this recently added parent directory hop: https://github.com/awslabs/llrt/commit/0ba28cd990ccf4d8b83ebf3c066b28ccc5cc77b8#diff-fb2489504fc47c00a43d4af72367d6cf617ec7caf0b7726062029c29366867a7R471

@KaanMol, I couldn't quite tell what the reason why the task root had to be moved one level higher, but to at least unblock the emulator, I just added an explicit instruction to set the task root.

If you think this is revealing a different problem, I'm happy to get some suggestions on how we can make sure this can work correctly when called from the root too

Checklist

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

KaanMol commented 1 month ago

Hi!

The reason had to do with it being a crate now, since it is in a subfolder, a few commands started failing. I don't exactly remember which ones, but there was a problem with something locally.

Adding this should be optionally imo, so I would classify this as a bug. Would you agree we rather solve this in the code, rather by adding docs for it?

nikp commented 1 month ago

Adding this should be optionally imo, so I would classify this as a bug. Would you agree we rather solve this in the code, rather by adding docs for it?

Definitely agreed! Can you recollect what failed? Just trying to think if we should be assuming get_task_root should be the current working directory or the root directory of the project (and what's the right way to determine the root..

KaanMol commented 1 month ago

Adding this should be optionally imo, so I would classify this as a bug. Would you agree we rather solve this in the code, rather by adding docs for it?

Definitely agreed! Can you recollect what failed? Just trying to think if we should be assuming get_task_root should be the current working directory or the root directory of the project (and what's the right way to determine the root..

Took me a while, but I found the initial reason why I changed that. If you change get_task_root back to this:

fn get_task_root() -> String {
    env::var(ENV_LAMBDA_TASK_ROOT).unwrap_or_else(|_| {
        env::current_dir()
            .unwrap_or("/".into())
            .into_os_string()
            .into_string()
            .unwrap()
    })
}

It will most likely resolve your issue, however, the command make test-ci will fail. The reason is probably because the test is ran by Cargo in the workspace folder which is llrt_core, instead of the project root. Thats why I added a parent there. Initially everything was under /src so it was never a problem.

To resolve this we can do a few things:

  1. Add the env variable to the Makefile only with this command
  2. Add it in the test of the runtime_client, by setting an env var or something.
  3. Modify runtime_client to make this less of a problem.

Is there one you would prefer, or do have another suggestion?

richarddavison commented 3 weeks ago

Fixed in https://github.com/awslabs/llrt/pull/406