cargo: auto detect DENO_BUILD_PATH #1134

piscisaureus · 2018-10-31T18:44:59Z

No description provided.

ry · 2018-10-31T18:55:53Z

build.rs

+
+ let deno_build_path = match env::var("DENO_BUILD_PATH") {
+ Ok(s) => repo_root_dir.join(s),
+ Err(_) => repo_root_dir.join("out").join(&mode),


This is inverting control. Because build.rs is running, it means we're a cargo build, but now we're ceding the output directory to how setup.py wants it. This is brittle because there are two place where "out/$mode" are hardcoded - setup.py and here.

We're just about to run tools/setup.py - why not tell it to gn gen into the directory we want it to. Cargo knows where it wants to put stuff, it has its own output directory. We should not invert control, but rather tell gn/ninja where to put build files.

We could put any path here: since we set DENO_BUILD_PATH when calling setup.py, it'll use that path no matter what it is.

However I prefer it to be the same because that allows me to use RLS without building v8 to a separate directory.

Although I do think it would make sense to make ninja build to OUT_DIR in some scenarios - maybe even the default - the "shared" setup has to be supported one way or another and it cannot be configured with an environment variable because I can't control the env vars that vscode passes to rls.

So until we add a config file, let's just land this.

I'm ok with landing this but it should be marked with a TODO - this is a hack to work around ccache and/or other problems with your setup - it's not the way it should be.

The desire to not build v8 in different directories is not a valid concern reason to invert build system control. If ccache doesn't work, do something hacky locally, like symlinking the directories.

I'm very concerned about not letting hacks get layered on top of hacks when interfacing with these build systems. We need to think carefully about what is happening - cargo is being run - it has created a directory for us to build into - we need to produce some static libraries so it can link to them. We have all the tools at our disposal to produce those static libraries in the provided OUT_DIR directory.

So until we add a config file, let's just land this.

I want to avoid any config files. We need to reduce the number of variables that control the build systems.

Another idea for a hack you can use locally: cargo check --target-dir out/debug

Another idea for a hack you can use locally: cargo check --target-dir out/debug

Again, I cannot control the invocation flow from vscode -> rls -> cargo.

@piscisaureus Then do it the other way,

./tools/setup --target-dir=`pwd`/target

There is nothing special about "out/"

Would it help you if we just renamed out/ to target/ ?

ry · 2018-10-31T19:21:18Z

build.rs

+ let repo_root_dir = Path::new(&repo_root_dir);
+
+ // TODO: use $OUT_DIR instead of "out/$PROFILE" as the default directory,
+ // once we properly skip the targets that are not needed by RLS.


"once we properly skip the targets that are not needed by RLS."

what does that mean?

I quote:

The RLS provides a server that runs in the background, providing IDEs, editors, and other tools with information about Rust programs. It supports functionality such as 'goto definition', symbol search, reformatting, and code completion, and enables renaming and refactorings.

RLS runs cargo check (or maybe some internal command similar to cargo check) to figure out the structure of the project. Repeatedly if things change that make it necessary.

It does not need v8 built, nor the snapshot, nor the bundle.
I would rather not build those targets every time i save a file. Not good for my mood and not good for the environment.

So build.rs should not build them.

Incremental builds are fast.

cargo unfortunately doesn't tell us if check or build is being run- from what I can tell. So we can't skip ninja.

cargo unfortunately doesn't tell us if check or build is being run- from what I can tell. So we can't skip ninja.

I'm sure there's something in here that gives it away:

CARGO_CFG_DEBUG_ASSERTIONS= CARGO_CFG_PROC_MACRO= CARGO_CFG_TARGET_ARCH=x86_64 CARGO_CFG_TARGET_ENDIAN=little CARGO_CFG_TARGET_ENV=msvc CARGO_CFG_TARGET_FAMILY=windows CARGO_CFG_TARGET_FEATURE=fxsr,sse,sse2 CARGO_CFG_TARGET_OS=windows CARGO_CFG_TARGET_POINTER_WIDTH=64 CARGO_CFG_WINDOWS= CARGO_HOME=C:\Users\BertBelder\.cargo CARGO_MAKEFLAGS=--jobserver-fds=__rust_jobserver_semaphore_1024240462 --jobserver-auth=__rust_jobserver_semaphore_1024240462 CARGO_MANIFEST_DIR=d:\deno2 CARGO_PKG_AUTHORS= CARGO_PKG_DESCRIPTION= CARGO_PKG_HOMEPAGE= CARGO_PKG_NAME=deno CARGO_PKG_VERSION_MAJOR=0 CARGO_PKG_VERSION_MINOR=0 CARGO_PKG_VERSION_PATCH=0 CARGO_PKG_VERSION_PRE= CARGO_PKG_VERSION=0.0.0 CARGO=\\?\C:\Users\BertBelder\.rustup\toolchains\stable-x86_64-pc-windows-msvc\bin\rls.exe LD_LIBRARY_PATH=C:\Users\BertBelder\.rustup\toolchains\stable-x86_64-pc-windows-msvc\lib RUST_RECURSION_COUNT=1 RUST_SRC_PATH=C:\Users\BertBelder\.rustup\toolchains\stable-x86_64-pc-windows-msvc/lib/rustlib/src/rust/src RUSTC=rustc RUSTDOC=rustdoc RUSTFLAGS= RUSTUP_HOME=C:\Users\BertBelder\.rustup RUSTUP_TOOLCHAIN=stable-x86_64-pc-windows-msvc VSCODE_CLI=1 VSCODE_CWD=D:\deno2 VSCODE_HANDLES_UNCAUGHT_ERRORS=true VSCODE_IPC_HOOK_EXTHOST=\\.\pipe\vscode-ipc-5570dc28-f150-4f53-8699-71a132190ec1-sock VSCODE_IPC_HOOK=\\.\pipe\a94eff2d73547178695cbd1dc4460f01-1.28.2-main-sock VSCODE_LOG_LEVEL=undefined VSCODE_LOG_STACK=false VSCODE_LOGS=C:\Users\BertBelder\AppData\Roaming\Code\logs\20181031T204117 VSCODE_NLS_CONFIG={"locale":"en-us","availableLanguages":{},"_languagePackSupport":true} VSCODE_NODE_CACHED_DATA_DIR_22888=C:\Users\BertBelder\AppData\Roaming\Code\CachedData\7f3ce96ff4729c91352ae6def877e59c561f4850 VSCODE_PID=22888

I've also checked - can't find anything.

ry · 2018-10-31T19:49:05Z

build.rs

 let mode = env::var("PROFILE").unwrap();
- let deno_build_path = env::var("DENO_BUILD_PATH").unwrap();
+
+ let repo_root_dir = env::var("CARGO_MANIFEST_DIR").unwrap();


Please add a TODO about removing usage of CARGO_MANIFEST_DIR. We should strive to reduce the amount of assumptions made about the build setup - in this case that we are able to write to this directory.

The directory is not assumed to be writable.

I'm using this as a rust equivalent of __dirname, so we can resolve DENO_BUILD_PATH iff it is a relative path. You could make DENO_BUILD_PATH absolute or set DENO_BUILD_PATH=../out/dir and it wouldn't try to write to the source dir at all.

Maybe what's confusing you is that rusts Path.join() behaves more like nodejs' path.resolve() and not like path.join().

ry

LGTM - it's fine - let's just move forward with caution and try to reduce hacks rather than add them.

ry · 2018-11-01T02:03:48Z

build.rs

+ let ninja_target = if check_only {
+ "cargo_check_deps"
+ } else {
+ "deno_deps"


ry

do not run flatc outside of gn

flatc is an implementation detail that cargo should not be exposed to. cargo only should see top-level targets like deno_deps. What's wrong with just building :msg_rs ?

piscisaureus · 2018-11-01T05:04:08Z

do not run flatc outside of gn

Nice job on the explaining the veto. Why is this?

ry

LGTM

ry · 2018-11-01T12:40:33Z

build.rs

+ println!("cargo:rustc-link-search=native={}/obj", gn_out_dir);
+ println!("cargo:rustc-link-lib=static=deno_deps");
+
+ println!("cargo:rerun-if-changed={}", abs_path("src/msg.fbs"));


This is getting dangerous. What is the behavior you’re trying to acheive / work around?
My goal is that this script be as dumb as possible and leave the build logic to our actual build scripts.

It solved my intellisense not updating after modifying the spawn message :)

It's still very broken of course -- any other change to libdeno or v8 or whatever dependency is ignored by cargo; it won't run build.rs so ninja never even gets the chance to rebuild it.

ry · 2018-11-01T12:41:38Z

build.rs


 let status = Command::new("python")
- .env("DENO_BUILD_PATH", &deno_build_path)
+ .env("DENO_BUILD_PATH", &gn_out_dir)
 .env("DENO_BUILD_MODE", &mode)
 .arg("./tools/setup.py")


I encourage you to remove this call before trying to optimize

ry · 2018-11-01T12:42:16Z

build.rs

 let status = Command::new("python")
- .env("DENO_BUILD_PATH", &deno_build_path)
+ .env("DENO_BUILD_PATH", &gn_out_dir)
 .env("DENO_BUILD_MODE", &mode)
 .arg("./tools/build.py")


Replace with direct call to ninja before trying to optimize further.

Plus some minor improvements and clean-ups: * Resolve DENO_BUILD_PATH to an absolute path if necessary. * Rename DENO_BUILD_PATH to GN_OUT_DIR in places where it is supposed to be set by the build system (and not a user configuration variable). * Output Cargo `rerun-if-*-changed` instructions first, so even if the build itself fails, configuration changes will still trigger a re-run of build.rs. * Remove TODOs that are impossible. * Re-run build.rs when the flatbuffer definition file changes.

piscisaureus · 2018-11-01T13:36:53Z

Enough tweaking!

ry · 2018-11-01T16:04:03Z

ref #695

.gitignore: dedupe

9aa3640

piscisaureus requested a review from ry October 31, 2018 18:44

piscisaureus force-pushed the cargo_auto branch from 74671db to 014beca Compare October 31, 2018 18:51

ry reviewed Oct 31, 2018

View reviewed changes

ry approved these changes Oct 31, 2018

View reviewed changes

ry approved these changes Nov 1, 2018

View reviewed changes

build.rs

let ninja_target = if check_only {

"cargo_check_deps"

} else {

"deno_deps"

Copy link

Member

ry Nov 1, 2018

Choose a reason for hiding this comment

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

👍

ry requested changes Nov 1, 2018

View reviewed changes

piscisaureus force-pushed the cargo_auto branch 10 times, most recently from 4513122 to ac0e2dd Compare November 1, 2018 12:15

piscisaureus added 2 commits November 1, 2018 13:34

build: remove absolute path

b73b651

cargo: build only necessary Ninja targets when invoked by RLS

67944f2

piscisaureus force-pushed the cargo_auto branch 2 times, most recently from 1fa3221 to 12e546c Compare November 1, 2018 12:37

ry approved these changes Nov 1, 2018

View reviewed changes

piscisaureus force-pushed the cargo_auto branch 2 times, most recently from f13b7ee to ae3d45e Compare November 1, 2018 13:18

piscisaureus force-pushed the cargo_auto branch from ae3d45e to ec17239 Compare November 1, 2018 13:18

piscisaureus merged commit ec17239 into denoland:master Nov 1, 2018

piscisaureus deleted the cargo_auto branch November 1, 2018 13:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cargo: auto detect DENO_BUILD_PATH #1134

cargo: auto detect DENO_BUILD_PATH #1134

piscisaureus commented Oct 31, 2018

ry Oct 31, 2018 •

edited

Loading

piscisaureus Oct 31, 2018 •

edited

Loading

ry Oct 31, 2018 •

edited

Loading

ry Oct 31, 2018

ry Oct 31, 2018

piscisaureus Oct 31, 2018

ry Oct 31, 2018 •

edited

Loading

ry Oct 31, 2018

ry Oct 31, 2018

piscisaureus Oct 31, 2018 •

edited

Loading

ry Oct 31, 2018

piscisaureus Oct 31, 2018

ry Oct 31, 2018

ry Oct 31, 2018

piscisaureus Oct 31, 2018

piscisaureus Oct 31, 2018

ry left a comment

ry Nov 1, 2018

ry left a comment •

edited

Loading

piscisaureus commented Nov 1, 2018

ry left a comment

ry Nov 1, 2018

piscisaureus Nov 1, 2018

ry Nov 1, 2018

ry Nov 1, 2018

piscisaureus commented Nov 1, 2018

ry commented Nov 1, 2018

cargo: auto detect DENO_BUILD_PATH #1134

cargo: auto detect DENO_BUILD_PATH #1134

Conversation

piscisaureus commented Oct 31, 2018

ry Oct 31, 2018 • edited Loading

Choose a reason for hiding this comment

piscisaureus Oct 31, 2018 • edited Loading

Choose a reason for hiding this comment

ry Oct 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ry Oct 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piscisaureus Oct 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ry left a comment • edited Loading

Choose a reason for hiding this comment

piscisaureus commented Nov 1, 2018

ry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piscisaureus commented Nov 1, 2018

ry commented Nov 1, 2018

ry Oct 31, 2018 •

edited

Loading

piscisaureus Oct 31, 2018 •

edited

Loading

ry Oct 31, 2018 •

edited

Loading

ry Oct 31, 2018 •

edited

Loading

piscisaureus Oct 31, 2018 •

edited

Loading

ry left a comment •

edited

Loading