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

Print gn args with -vv option #1845

Merged
merged 4 commits into from Mar 2, 2019

Conversation

2 participants
@kt3k
Copy link
Contributor

kt3k commented Feb 27, 2019

This PR addresses #1328.

With this change, you can pass -vv (double --version) option to deno, and it prints like the below:

$ ./target/debug/deno -vv
deno: 0.3.0
v8: 7.4.158
typescript: 3.2.1

cc_wrapper = "/Users/kt3k/s/deno/prebuilt/mac/sccache"
clang_use_chrome_plugins = false
is_cfi = false
is_component_build = false
is_debug = true
is_desktop_linux = false
proprietary_codecs = false
rust_treat_warnings_as_errors = true
rustc_wrapper = "/Users/kt3k/s/deno/prebuilt/mac/sccache"
safe_browsing_mode = 0
symbol_level = 1
toolkit_views = false
treat_warnings_as_errors = true
use_aura = false
use_dbus = false
use_gio = false
use_glib = false
use_jumbo_build = true
use_ozone = false
use_udev = false
v8_deprecation_warnings = false
v8_enable_gdbjit = false
v8_enable_i18n_support = false
v8_extra_library_files = []
v8_imminent_deprecation_warnings = false
v8_monolithic = false
v8_postmortem_support = true
v8_untrusted_code_mitigations = false
v8_use_external_startup_data = false
v8_use_snapshot = true

closes #1328

@@ -20,6 +20,7 @@ const typescriptPath = path.resolve(
__dirname,
"third_party/node_modules/typescript/lib/typescript.js"
);
const gnArgs = fs.readFileSync("args_list.txt", "utf-8").trim();

This comment has been minimized.

@ry

ry Feb 27, 2019

Collaborator

@kt3k The patch looks good except for where you write 'args_list.txt' in 'setup.py'. It think this should be done differently - without using setup.py.

Here's an outline of what I think should happen:

  1. I think you should use exec_script to generate args_list.txt into a string. Similar to this

    deno/BUILD.gn

    Lines 135 to 138 in 33a6409

    # Reads the cargo info from Cargo.toml
    deno_cargo_info = exec_script("build_extra/rust/get_cargo_info.py",
    [ rebase_path("Cargo.toml", root_build_dir) ],
    "json")
  2. This string should be passed to "main_bundle" here

    deno/BUILD.gn

    Lines 216 to 223 in 33a6409

    bundle("main_bundle") {
    out_dir = "$target_gen_dir/bundle/"
    out_name = "main"
    deps = [
    ":deno_runtime_declaration",
    ":msg_ts",
    ]
    }
    as a new variable.. something like gn_args = output_from_exec_script
  3. You must then add something to the bundle template, to take "gn_args" and pass it to the rollup program via command line:

    deno/libdeno/deno.gni

    Lines 4 to 31 in 33a6409

    # Tempalte to generate a Rollup bundle of code.
    template("bundle") {
    action(target_name) {
    forward_variables_from(invoker, "*")
    script = "//tools/run_node.py"
    outputs = [
    out_dir + out_name + ".js",
    out_dir + out_name + ".js.map",
    ]
    inputs = [
    "js/" + out_name + ".ts",
    "rollup.config.js",
    ]
    depfile = out_dir + out_name + ".d"
    args = [
    rebase_path("third_party/node_modules/rollup/bin/rollup", root_build_dir),
    "-c",
    rebase_path("rollup.config.js", root_build_dir),
    "-i",
    rebase_path(inputs[0], root_build_dir),
    "-o",
    rebase_path(outputs[0], root_build_dir),
    "--sourcemapFile",
    rebase_path("."),
    "--silent",
    ]
    }
    }

This comment has been minimized.

@kt3k

kt3k Feb 28, 2019

Author Contributor

OK. I'll try this approach.

This comment has been minimized.

@kt3k

kt3k Feb 28, 2019

Author Contributor

Steps 2 and 3 seem feasible, but step 1 doesn't.

When I put gn args command in exec_script like:

deno_build_args = exec_script("third_party/depot_tools/gn.py",
                              [ "args", root_build_dir, "--list", "--short" ],
                              "string")

Then it errored like the below:

/Users/kt3k/s/deno/third_party/depot_tools/gn gen /Users/kt3k/s/deno/target/release
ERROR at //BUILD.gn:140:19: Script returned non-zero exit code.
deno_cargo_info = exec_script("third_party/depot_tools/gn.py",
                  ^----------
Current dir: /Users/kt3k/s/deno/target/release/
Command: python /Users/kt3k/s/deno/third_party/depot_tools/gn.py args //target/release --list --short
Returned 1 and printed out:

ERROR Not a build directory.
This command requires an existing build directory. I interpreted your input
"//target/release" as:
  /Users/kt3k/s/deno/target/release/
which doesn't seem to contain a previously-generated build.

I think this is because at this stage gn gen is not finished and build directory is not ready. And that causes gn args to fail. So I think we need to finish the entire gn gen before running gn args, and that means we can't put gn args command in BUILD.gn.

This comment has been minimized.

@ry

ry Feb 28, 2019

Collaborator

Ok, that makes sense, but I think an action will work. Here's an (untested) attempt:

  action("write_gn_args") {
    script = "//tools/write_gn_args.py"
    args = [ rebase_path(outputs[0], root_build_dir) ]
    outputs = [ "$target_gen_dir/gn_args.txt" ]
  }

you'll add write_gn_args to the deps section of rollup action.

This comment has been minimized.

@kt3k

kt3k Mar 1, 2019

Author Contributor

Thanks! That worked!

@@ -159,7 +163,7 @@ pub fn set_flags(
opts.optflag("", "recompile", "Force recompilation of TypeScript code");
opts.optflag("h", "help", "Print this message");
opts.optflag("D", "log-debug", "Log debug output");
opts.optflag("v", "version", "Print the version");
opts.optflagmulti("v", "version", "Print the version.");

This comment has been minimized.

@ry

ry Feb 27, 2019

Collaborator

I'm also not sure about -vv ... It seems very specific.

I like that we can access this info at runtime via Deno.versions.gnArgs (maybe rename this to Deno.versions.buildArgs?) ... And I think this is information that most people will not care about.

If we had some way of doing node -pe in deno, then we wouldn't need to add a new command-line argument, we could just tell people to to do deno -pe Deno.versions.gnArgs. So I'd say for now let's not expose this to the command line, but force people to access it via Deno.versions.gnArgs.

(I'm not so happy with how -pe was designed... We should probably discuss in a separate issue.)

This comment has been minimized.

@ry

ry Feb 28, 2019

Collaborator

Maybe we can combine Deno.versions and Deno.platform into Deno.build ?

Deno.build.denoVersion
Deno.build.v8Version
Deno.build.typescriptVersion
Deno.build.arch
Deno.build.os
Deno.build.gnArgs

This comment has been minimized.

@kt3k

kt3k Mar 1, 2019

Author Contributor

That's possible, but I think that involves some breaking changes, and I'm not sure that's worth it. (Deno.platform seems being used a few time in deno_std https://github.com/denoland/deno_std/search?q=platform&unscoped_q=platform )

This comment has been minimized.

@ry

ry Mar 2, 2019

Collaborator

I'd rather have breaking changes now than in 6 months. People have been warned.

I think Deno.versions.gnArgs is not the most semantically meaningful statement... I've added an issue for it #1865

This comment has been minimized.

@kt3k

kt3k Mar 2, 2019

Author Contributor

Thanks for making an issue. I'll work on this when the members reach a consensus.

@kt3k kt3k force-pushed the kt3k:feature/print-gn-args branch from 944e5bc to f802b48 Feb 28, 2019

@@ -219,6 +219,7 @@ bundle("main_bundle") {
deps = [

This comment has been minimized.

@ry

ry Mar 1, 2019

Collaborator

Add data = [ "$target_gen_dir/gn_args.txt" ]

(if it doesn't work, at least add a TODO to do it)

@kt3k kt3k force-pushed the kt3k:feature/print-gn-args branch 2 times, most recently from 61a5c3f to 35c4024 Mar 1, 2019

@kt3k kt3k force-pushed the kt3k:feature/print-gn-args branch from 35c4024 to 5db61ec Mar 1, 2019

@ry

ry approved these changes Mar 2, 2019

Copy link
Collaborator

ry left a comment

LGTM - nice feature - thanks!

@ry ry merged commit a7bb8cc into denoland:master Mar 2, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@kt3k kt3k deleted the kt3k:feature/print-gn-args branch Mar 2, 2019

@kt3k kt3k referenced this pull request Mar 2, 2019

Merged

Fix race condition in build #1870

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.