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

build: allow disabling snapshots for dev #20048

Merged
merged 10 commits into from
Aug 5, 2023

Conversation

nayeemrmn
Copy link
Collaborator

Closes #19399 (running without snapshots at all was suggested as an alternative solution).

Adds a __runtime_js_sources pseudo-private feature to load extension JS sources at runtime for faster development, instead of building and loading snapshots or embedding sources in the binary. Will only work in a development environment obviously.

Try running cargo test --features __runtime_js_sources integration::node_unit_tests::os_test. Then break some behaviour in ext/node/polyfills/os.ts e.g. make function cpus() {} return an empty array, and run it again. Fix and then run again. No more build time in between.

Blocked by #20043.

cc @bartlomieju

@bartlomieju
Copy link
Member

bartlomieju commented Aug 4, 2023

@nayeemrmn fantastic! I think I managed to find an edge case here.

If I do:

$ cargo build --features __runtime_js_sources

And then:

$ ./target/debug/deno

I get:

./target/debug/deno

============================================================
Deno has panicked. This is a bug in Deno. Please report this
at https://github.com/denoland/deno/issues/new.
If you can reliably reproduce this panic, include the
reproduction steps and re-run with the RUST_BACKTRACE=1 env
var set and include the backtrace in your report.

Platform: macos aarch64
Version: 1.36.0
Args: ["./target/debug/deno"]

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: NotPresent', ext/node/lib.rs:136:6
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

The panic doesn't appear if I do cargo run --features __runtime_js_sources. Any ideas?

EDIT: The same problem happens if I actually run a script:

// foo.js
import process from "node:process";

console.log("process.arch", process.arch);
$ cargo run --features __runtime_js_sources -- run foo.js
process.arch arm64
$ ./target/debug/deno run foo.js
============================================================
Deno has panicked. This is a bug in Deno. Please report this
at https://github.com/denoland/deno/issues/new.
If you can reliably reproduce this panic, include the
reproduction steps and re-run with the RUST_BACKTRACE=1 env
var set and include the backtrace in your report.

Platform: macos aarch64
Version: 1.36.0
Args: ["./target/debug/deno", "run", "foo.js"]

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: NotPresent', ext/node/lib.rs:136:6
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@nayeemrmn
Copy link
Collaborator Author

nayeemrmn commented Aug 4, 2023

Strange, I'm not getting that error. From a quick glance at

deno/ext/node/lib.rs

Lines 135 to 136 in 5311f69

std::env::var("TARGET")
.unwrap()
why is it reading TARGET at runtime instead of env!()?

Ah yeah makes sense of course that won't work given what this mode is.

@bartlomieju
Copy link
Member

Strange, I'm not getting that error. From a quick glance at

deno/ext/node/lib.rs

Lines 135 to 136 in 5311f69

std::env::var("TARGET")
.unwrap()

why is it reading TARGET at runtime instead of env!()?

This is the diagnostic I get if I try to use env!:

environment variable `TARGET` not defined at compile time
Cargo sets build script variables at run time. Use `std::env::var("TARGET")` instead

@bartlomieju
Copy link
Member

It's strange you don't get that error - this op is called in 3 separate places by ext/node so you should definitely be calling it if executing some node: built-in module or npm: package.

@nayeemrmn
Copy link
Collaborator Author

Okay it's because the TARGET is defined in runtime/build.rs,deno_node is build before that.

@bartlomieju
Copy link
Member

Okay it's because the TARGET is defined in runtime/build.rs,deno_node is build before that.

We should probably move all of that to deno_core and have build info attached there to Deno.core.build. We are currently using these env vars in several places :/

@bartlomieju
Copy link
Member

@nayeemrmn this is absolutely fantastic. I've been playing with it for half an hour now and it's a game changer in terms of working on the JS APIs. I love how few changes you had to do, I'll release deno_core tonight after Matt finishes some stuff so we can land it ASAP.

@marvinhagemeister
Copy link
Contributor

Just wanted to say that this is fantastic feature! Really looking forward to seeing this sometime in Deno 👍

@nayeemrmn
Copy link
Collaborator Author

nayeemrmn commented Aug 4, 2023

$ ./target/debug/deno

By the way I could reproduce the error with target/debug/deno, using cargo run seems to initialise the cargo:rustc-env values on startup (for me only, still strange). Anyway I added target-lexicon from bytecodealliance to get the target triple at compile time ext/node/build.rs to set the var.

Just wanted to say that this is fantastic feature! Really looking forward to seeing this sometime in Deno 👍

Thanks!

ext/node/lib.rs Outdated Show resolved Hide resolved
@sigmaSd
Copy link
Contributor

sigmaSd commented Aug 4, 2023

does this have any effect on cross compiling, as in can I now crosscompile by disabling snapshotting ?

@nayeemrmn
Copy link
Collaborator Author

does this have any effect on cross compiling, as in can I now crosscompile by disabling snapshotting ?

No, this snapshot-less mode creates a binary that only works on the development machine it was built on. That would require an alternative snapshot-less mode which bundles all the sources in the binary.

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM, fantastic work @nayeemrmn, this will make working on JS/TS polyfills way faster! Kudos 🚀

@bartlomieju bartlomieju changed the title test: allow disabling snapshots for dev build: allow disabling snapshots for dev Aug 5, 2023
@bartlomieju bartlomieju merged commit c1c8eb3 into denoland:main Aug 5, 2023
13 checks passed
@nayeemrmn nayeemrmn deleted the dev-runtime-js-sources branch August 6, 2023 00:22
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.

Ability to create and load a snapshot without rebuilding the whole binary
4 participants