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

feat(compile): Enable multiple roots for a standalone module graph #17663

Merged
merged 9 commits into from
Mar 18, 2023
22 changes: 22 additions & 0 deletions cli/args/flags.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ pub struct CompileFlags {
pub output: Option<PathBuf>,
pub args: Vec<String>,
pub target: Option<String>,
pub side_modules: Vec<String>,
}

#[derive(Clone, Debug, Eq, PartialEq)]
Expand Down Expand Up @@ -906,6 +907,20 @@ fn compile_subcommand<'a>() -> Command<'a> {
runtime_args(Command::new("compile"), true, false)
.trailing_var_arg(true)
.arg(script_arg().required(true))
.arg(
Arg::new("side-module")
andreubotella marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was flag name settled? Me and @aapoalas suggested --import which is less colloquial than --side-module.

Copy link
Member

Choose a reason for hiding this comment

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

No - it's still up to debate/bike-shedding, but I'm fine changing the name in a follow up. I'm not sure I like --import better than --side-module, seems more ambigious.

Copy link
Member

Choose a reason for hiding this comment

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

--include ? Shorter is better here...

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with --include too

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah --include was our next one #17663 (comment), sounds perfect if it might include assets as well.

Copy link
Member

Choose a reason for hiding this comment

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

ok let's switch to --include

Copy link
Contributor Author

@andreubotella andreubotella Mar 17, 2023

Choose a reason for hiding this comment

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

As far as I'm aware, including assets in the binary is a very different thing than including a module in the module graph. Though I guess you could reconstruct the original TS file from the source map.

In any case, if you agree that the flag name should be --include, I'm fine with that.

.long("side-module")
.help("Additional module to include in the module graph")
bartlomieju marked this conversation as resolved.
Show resolved Hide resolved
.long_help(
"Includes an additional module in the compiled executable's module \
graph. Use this flag if a dynamically imported module or a web worker main \
module fails to load in the executable. This flag can be passed multiple \
times, to include multiple additional modules.",
Comment on lines +917 to +920
Copy link
Member

Choose a reason for hiding this comment

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

Can I do comma separated list of urls like: --side-module=https://deno.land/std/flags/mod.ts,https://deno.land/std/version.ts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Commas are valid in URLs and filenames.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, I thought we already supported comma separated lists in other flags, but maybe I'm wrong here

)
.takes_value(true)
.multiple_occurrences(true)
andreubotella marked this conversation as resolved.
Show resolved Hide resolved
.value_hint(ValueHint::FilePath),
)
.arg(
Arg::new("output")
.long("output")
Expand Down Expand Up @@ -2484,12 +2499,17 @@ fn compile_parse(flags: &mut Flags, matches: &clap::ArgMatches) {
let source_file = script[0].to_string();
let output = matches.value_of("output").map(PathBuf::from);
let target = matches.value_of("target").map(String::from);
let side_modules = match matches.values_of("side-module") {
Some(f) => f.map(String::from).collect(),
None => vec![],
};

flags.subcommand = DenoSubcommand::Compile(CompileFlags {
source_file,
output,
args,
target,
side_modules,
});
}

Expand Down Expand Up @@ -6240,6 +6260,7 @@ mod tests {
output: None,
args: vec![],
target: None,
side_modules: vec![]
}),
type_check_mode: TypeCheckMode::Local,
..Flags::default()
Expand All @@ -6259,6 +6280,7 @@ mod tests {
output: Some(PathBuf::from("colors")),
args: svec!["foo", "bar"],
target: None,
side_modules: vec![]
}),
import_map_path: Some("import_map.json".to_string()),
no_remote: true,
Expand Down
4 changes: 2 additions & 2 deletions cli/graph_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ pub fn graph_lock_or_exit(graph: &ModuleGraph, lockfile: &mut Lockfile) {
}

pub async fn create_graph_and_maybe_check(
bartlomieju marked this conversation as resolved.
Show resolved Hide resolved
root: ModuleSpecifier,
roots: Vec<ModuleSpecifier>,
ps: &ProcState,
) -> Result<Arc<deno_graph::ModuleGraph>, AnyError> {
let mut cache = cache::FetchCacher::new(
Expand All @@ -176,7 +176,7 @@ pub async fn create_graph_and_maybe_check(
build_graph_with_npm_resolution(
&mut graph,
&ps.npm_resolver,
vec![root],
roots,
&mut cache,
deno_graph::BuildOptions {
is_dynamic: false,
Expand Down
34 changes: 34 additions & 0 deletions cli/tests/integration/compile_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -592,3 +592,37 @@ fn dynamic_import() {
.unwrap();
assert_eq!(String::from_utf8(output.stdout).unwrap(), expected);
}

#[test]
fn dynamic_import_unanalyzable() {
let _guard = util::http_server();
let dir = TempDir::new();
let exe = if cfg!(windows) {
dir.path().join("dynamic_import_unanalyzable.exe")
} else {
dir.path().join("dynamic_import_unanalyzable")
};
let output = util::deno_cmd()
.current_dir(util::root_path())
.arg("compile")
.arg("--allow-read")
.arg("--side-module")
.arg(util::testdata_path().join("./compile/dynamic_imports/import1.ts"))
andreubotella marked this conversation as resolved.
Show resolved Hide resolved
.arg("--output")
.arg(&exe)
.arg(
util::testdata_path()
.join("./compile/dynamic_imports/main_unanalyzable.ts"),
)
.output()
.unwrap();
assert!(output.status.success());

let output = Command::new(&exe).env("NO_COLOR", "").output().unwrap();
assert!(output.status.success());
let expected = std::fs::read_to_string(
util::testdata_path().join("./compile/dynamic_imports/main.out"),
)
.unwrap();
assert_eq!(String::from_utf8(output.stdout).unwrap(), expected);
}
1 change: 1 addition & 0 deletions cli/tests/testdata/compile/dynamic_imports/import_path
andreubotella marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
./import1.ts
18 changes: 18 additions & 0 deletions cli/tests/testdata/compile/dynamic_imports/main_unanalyzable.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import { join } from "https://deno.land/[email protected]/path/mod.ts";

console.log("Starting the main module");

// We load the dynamic import path from the file system, to make sure any
// improvements in static analysis can't defeat the purpose of this test, which
// is to make sure the `--site-module` flag works to add non-analyzed imports
// to the module graph.
const IMPORT_PATH_FILE_PATH = join(
Deno.cwd(),
"tests/testdata/compile/dynamic_imports/import_path",
);

setTimeout(async () => {
console.log("Dynamic importing");
const importPath = (await Deno.readTextFile(IMPORT_PATH_FILE_PATH)).trim();
import(importPath).then(() => console.log("Dynamic import done."));
}, 500);
bartlomieju marked this conversation as resolved.
Show resolved Hide resolved
andreubotella marked this conversation as resolved.
Show resolved Hide resolved
3 changes: 2 additions & 1 deletion cli/tools/bundle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@ pub async fn bundle(
log::debug!(">>>>> bundle START");
let ps = ProcState::from_options(cli_options).await?;
let graph =
create_graph_and_maybe_check(module_specifier.clone(), &ps).await?;
create_graph_and_maybe_check(vec![module_specifier.clone()], &ps)
.await?;

let mut paths_to_watch: Vec<PathBuf> = graph
.specifiers()
Expand Down
17 changes: 13 additions & 4 deletions cli/tools/standalone.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,14 @@ pub async fn compile(
let ps = ProcState::build(flags).await?;
let module_specifier =
resolve_url_or_path(&compile_flags.source_file, ps.options.initial_cwd())?;
let module_roots = {
let mut vec = Vec::with_capacity(compile_flags.side_modules.len() + 1);
vec.push(module_specifier.clone());
for side_module in &compile_flags.side_modules {
vec.push(resolve_url_or_path(side_module, ps.options.initial_cwd())?);
}
vec
};
let deno_dir = &ps.dir;

let output_path = resolve_compile_executable_output_path(
Expand All @@ -49,10 +57,9 @@ pub async fn compile(
)
.await?;

let graph = Arc::try_unwrap(
create_graph_and_maybe_check(module_specifier.clone(), &ps).await?,
)
.unwrap();
let graph =
Arc::try_unwrap(create_graph_and_maybe_check(module_roots, &ps).await?)
.unwrap();

// at the moment, we don't support npm specifiers in deno_compile, so show an error
error_for_any_npm_specifier(&graph)?;
Expand Down Expand Up @@ -351,6 +358,7 @@ mod test {
output: Some(PathBuf::from("./file")),
args: Vec::new(),
target: Some("x86_64-unknown-linux-gnu".to_string()),
side_modules: vec![],
},
&std::env::current_dir().unwrap(),
)
Expand All @@ -371,6 +379,7 @@ mod test {
output: Some(PathBuf::from("./file")),
args: Vec::new(),
target: Some("x86_64-pc-windows-msvc".to_string()),
side_modules: vec![],
},
&std::env::current_dir().unwrap(),
)
Expand Down