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

deno_core prints cargo related stuff if used as a dependency #11030

Closed
maxxcs opened this issue Jun 18, 2021 · 9 comments · Fixed by #14614
Closed

deno_core prints cargo related stuff if used as a dependency #11030

maxxcs opened this issue Jun 18, 2021 · 9 comments · Fixed by #14614
Labels
bug Something isn't working correctly deno_core Changes in "deno_core" crate are needed

Comments

@maxxcs
Copy link
Contributor

maxxcs commented Jun 18, 2021

After #10786 I'm getting cargo:rerun-if-changed=.../core/core.js and cargo:rerun-if-changed=.../core/error.js printed on the console when I have deno_core as a dependency, even in release builds.

@lucacasonato
Copy link
Member

The reason you are seeing this is because you are not snapshotting your code. Snapshotting is required. For an example of how to use runtime snapshotting, see https://github.com/lucacasonato/wgpu_cts_runner/blob/main/build.rs and https://github.com/lucacasonato/wgpu_cts_runner/blob/main/src/main.rs.

@kvark
Copy link

kvark commented Sep 14, 2021

@lucacasonato can we revive this? wgpu's cts_runner isn't snapshotting either, and we'd very much prefer not to have this spam in the logs.

@bartlomieju bartlomieju reopened this Sep 14, 2021
@stale
Copy link

stale bot commented Nov 13, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Nov 13, 2021
@kvark
Copy link

kvark commented Nov 15, 2021

Unless this is fixed, it's still something we want.

@stale stale bot removed the stale label Nov 15, 2021
@lucacasonato lucacasonato added deno_core Changes in "deno_core" crate are needed bug Something isn't working correctly labels Nov 15, 2021
@nikita-skobov
Copy link

@lucacasonato I was getting the following output when running the hello world deno_core example:

cargo:rerun-if-changed=/home/madmin/Desktop/projects/deno/core/00_primordials.js
cargo:rerun-if-changed=/home/madmin/Desktop/projects/deno/core/01_core.js
cargo:rerun-if-changed=/home/madmin/Desktop/projects/deno/core/02_error.js
The sum of
1,2,3
is
6
Exception:
Error: Error parsing args: serde_v8 error: ExpectedArray

I think the cargo:rerun stuff should not be there, right?

I think it is because of the following two places:

deno/core/ops_builtin.rs

Lines 20 to 25 in 7ebbda7

.js(include_js_files!(
prefix "deno:core",
"00_primordials.js",
"01_core.js",
"02_error.js",
))

and:

println!("cargo:rerun-if-changed={}", path.display());

I don't know enough about deno's architecture to know why it's required, but is there a way to disable this? ie: a feature flag or something?

Ideally, if I create an executable that uses deno_core, I would like to not have that cargo:rerun output.

@bartlomieju
Copy link
Member

This print should only happen during "build" step (cargo build, cargo check, etc) and not during actual execution. We need to find a way to tell if the related macro is invoked during build step or during bootstrapping of runtime when executing built target.

@nikita-skobov
Copy link

This print should only happen during "build" step

I could be wrong but I think the issue is as follows:

The include_js_files macro is used by other parts of deno, and indeed is required for the build step.

However, within deno core, the only time that the include_js_files macro is called is from the init_builtins() function, which is only ever called from JsRuntime::new() which can only happen at runtime, if im not mistaken.

I think a potential solution is to create a separate version of this include_js_files macro that is used specifically by that init_builtins() function, and the rest of deno can use the original version of include_js_files!

@andreubotella
Copy link
Contributor

andreubotella commented Dec 26, 2021

I was looking into this the other day: there are a number of environment variables that are defined for build scripts (and presumably, their dependencies), but not elsewhere (https://doc.rust-lang.org/cargo/reference/environment-variables.html#environment-variables-cargo-sets-for-build-scripts). So you could switch based on those envvars with option_env!.

The problem is that the non-build case would have to use include_str! to make the binary usable on other machines, as it used to be before #10786. But the performance issue with include_str! that #10786 fixed happens if the macro is used at all in compiled code, so we would need some conditional compilation, which option_env! does not do.

@andreubotella
Copy link
Contributor

Or, come to think of it, we could have a build-time-snapshot feature (name to be bikeshedded) that deno_runtime and the CLI crate would enable. We could then throw a compile-time error if you try to use that feature outside of a build script.

andreubotella pushed a commit to andreubotella/deno that referenced this issue Dec 29, 2021
andreubotella added a commit to andreubotella/deno that referenced this issue Mar 14, 2022
AaronO added a commit that referenced this issue May 15, 2022
This reverts commit 10e50a1

Alternative to #13217, IMO the tradeoffs made by #10786 aren't worth it.

It breaks abstractions (crates being self-contained, deno_core without snapshotting etc...) and causes pain points / gotchas for both embedders & devs for a relatively minimal gain in incremental build time ...

Closes #11030
sigmaSd pushed a commit to sigmaSd/deno that referenced this issue May 29, 2022
…enoland#14614)

This reverts commit 10e50a1

Alternative to denoland#13217, IMO the tradeoffs made by denoland#10786 aren't worth it.

It breaks abstractions (crates being self-contained, deno_core without snapshotting etc...) and causes pain points / gotchas for both embedders & devs for a relatively minimal gain in incremental build time ...

Closes denoland#11030
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working correctly deno_core Changes in "deno_core" crate are needed
Projects
None yet
6 participants