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

perf: skip npm install if graph has no new packages #24017

Merged
merged 24 commits into from
May 28, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
More improvements
  • Loading branch information
dsherret committed May 27, 2024
commit 2b0192f8c6bfecb3706aae26cb0a2f4b7c4c3700
8 changes: 4 additions & 4 deletions .dprint.json
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,10 @@
"ext/websocket/autobahn/reports"
],
"plugins": [
"https://plugins.dprint.dev/typescript-0.90.5.wasm",
"https://plugins.dprint.dev/json-0.19.2.wasm",
"https://plugins.dprint.dev/markdown-0.17.0.wasm",
"https://plugins.dprint.dev/toml-0.6.1.wasm",
"https://plugins.dprint.dev/typescript-0.91.0.wasm",
"https://plugins.dprint.dev/json-0.19.3.wasm",
"https://plugins.dprint.dev/markdown-0.17.1.wasm",
"https://plugins.dprint.dev/toml-0.6.2.wasm",
"https://plugins.dprint.dev/exec-0.4.4.json@c207bf9b9a4ee1f0ecb75c594f774924baf62e8e53a2ce9d873816a408cecbf7"
]
}
6 changes: 1 addition & 5 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -373,5 +373,7 @@ opt-level = 3
[patch.crates-io]
deno_doc = { path = "../deno_doc" }
deno_graph = { path = "../deno_graph" }
eszip = { path = "../eszip" }
deno_emit = { path = "../deno_emit/rs-lib" }
deno_unsync = { path = "../deno_unsync" }
deno_lockfile = { path = "../deno_lockfile" }
2 changes: 1 addition & 1 deletion cli/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ deno_config = "=0.16.3"
deno_core = { workspace = true, features = ["include_js_files_for_snapshotting"] }
deno_doc = { version = "=0.136.0", features = ["html", "syntect"] }
deno_emit = "=0.40.3"
deno_graph = { version = "=0.75.2", features = ["tokio_executor"] }
deno_graph = { version = "=0.76.0", features = ["tokio_executor"] }
deno_lint = { version = "=0.58.4", features = ["docs"] }
deno_lockfile.workspace = true
deno_npm = "=0.20.2"
Expand Down
1 change: 1 addition & 0 deletions cli/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ fn get_module_graph_error_class(err: &ModuleGraphError) -> &'static str {
| JsrLoadError::ContentLoad(_)
| JsrLoadError::ContentChecksumIntegrity(_)
| JsrLoadError::PackageManifestLoad(_, _)
| JsrLoadError::PackageVersionManifestChecksumIntegrity(..)
| JsrLoadError::PackageVersionManifestLoad(_, _)
| JsrLoadError::RedirectInPackage(_) => "Error",
JsrLoadError::PackageNotFound(_)
Expand Down
153 changes: 118 additions & 35 deletions cli/graph_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ use crate::tools::check::TypeChecker;
use crate::util::file_watcher::WatcherCommunicator;
use crate::util::fs::canonicalize_path;
use deno_emit::LoaderChecksum;
use deno_graph::JsrLoadError;
use deno_graph::ModuleLoadError;
use deno_runtime::fs_util::specifier_to_file_path;

use deno_config::WorkspaceMemberConfig;
Expand Down Expand Up @@ -96,8 +98,71 @@ pub fn graph_valid(
enhanced_resolution_error_message(resolution_error)
)
}
ModuleGraphError::ModuleError(e) => {
enhanced_module_error_message(fs.clone(), e)
ModuleGraphError::ModuleError(error) => {
match error {
ModuleError::LoadingErr(specifier, _, ModuleLoadError::Loader(_)) // ex. "Is a directory" error
| ModuleError::Missing(specifier, _) => {
sloppy_imports_error_message(fs.clone(), specifier, error)
}
ModuleError::LoadingErr(specifier, _, ModuleLoadError::Jsr(JsrLoadError::ContentChecksumIntegrity(checksum_err))) => {
let err = format!(
concat!(
"Integrity check failed in package. The package may have been tampered with.\n\n",
"Specifier: {}\n",
"Actual: {}\n",
"Expected: {}\n\n",
"If you modified your global cache, run again with the --reload ",
"flag to restore its state. If you want to modify dependencies ",
"locally run again with the --vendor flag or specify \"vendor\": true` ",
"in a deno.json then modify the contents of the `vendor` folder."
),
specifier,
checksum_err.actual,
checksum_err.expected,
);
log::error!("{} {}", colors::red("error:"), err);
std::process::exit(10);
}
ModuleError::LoadingErr(_specifier, _, ModuleLoadError::Jsr(JsrLoadError::PackageVersionManifestChecksumIntegrity(package_nv, checksum_err))) => {
let err = format!(
concat!(
"Integrity check failed for package. The source code is invalid, as it does not match the expected hash in the lock file.\n\n",
" Package: {}\n",
" Actual: {}\n",
" Expected: {}\n\n",
"This could be caused by:\n",
" * the lock file may be corrupt\n",
" * the source itself may be corrupt\n\n",
"Use the --lock-write flag to regenerate the lockfile or --reload to reload the source code from the server."
),
package_nv,
checksum_err.actual,
checksum_err.expected,
);
log::error!("{} {}", colors::red("error:"), err);
std::process::exit(10);
}
ModuleError::LoadingErr(specifier, _, ModuleLoadError::HttpsChecksumIntegrity(checksum_err)) => {
let err = format!(
concat!(
"Integrity check failed for remote specifier. The source code is invalid, as it does not match the expected hash in the lock file.\n\n",
" Specifier: {}\n",
" Actual: {}\n",
" Expected: {}\n\n",
"This could be caused by:\n",
" * the lock file may be corrupt\n",
" * the source itself may be corrupt\n\n",
"Use the --lock-write flag to regenerate the lockfile or --reload to reload the source code from the server."
),
specifier,
checksum_err.actual,
checksum_err.expected,
);
log::error!("{} {}", colors::red("error:"), err);
std::process::exit(10);
}
_ => format!("{}", error),
}
}
};

Expand Down Expand Up @@ -384,6 +449,46 @@ impl ModuleGraphBuilder {
}
}

// todo(THIS PR): maybe clone from the lockfile then repopulate
struct LockfileLocker(Arc<Mutex<Lockfile>>);

impl deno_graph::source::Locker for LockfileLocker {
fn get_checksum(
&self,
specifier: &deno_ast::ModuleSpecifier,
) -> Option<LoaderChecksum> {
self
.0
.lock()
.content
.remote
.get(specifier.as_str())
.map(|s| LoaderChecksum::new(s.clone()))
}

fn has_checksum(&self, specifier: &deno_ast::ModuleSpecifier) -> bool {
self
.0
.lock()
.content
.remote
.contains_key(specifier.as_str())
}

fn set_checksum(
&mut self,
specifier: &deno_ast::ModuleSpecifier,
checksum: LoaderChecksum,
) {
self
.0
.lock()
.content
.remote
.insert(specifier.as_str().to_string(), checksum.into_string());
}
}

let maybe_imports = self.options.to_maybe_imports()?;
let analyzer = self
.module_info_cache
Expand All @@ -401,6 +506,10 @@ impl ModuleGraphBuilder {
.map(|r| r.as_reporter());
let workspace_members =
self.options.resolve_deno_graph_workspace_members()?;
let mut locker = self
.lockfile
.as_ref()
.map(|lockfile| LockfileLocker(lockfile.clone()));
self
.build_graph_with_npm_resolution_and_build_options(
graph,
Expand All @@ -418,7 +527,7 @@ impl ModuleGraphBuilder {
module_analyzer: &analyzer,
reporter: maybe_file_watcher_reporter,
resolver: Some(graph_resolver),
verify_and_fill_checksums: self.lockfile.is_some(),
locker: locker.as_mut().map(|l| l as _),
},
)
.await
Expand All @@ -428,7 +537,7 @@ impl ModuleGraphBuilder {
&self,
graph: &mut ModuleGraph,
roots: Vec<ModuleSpecifier>,
loader: &mut dyn deno_graph::source::Loader,
loader: &'a mut dyn deno_graph::source::Loader,
options: deno_graph::BuildOptions<'a>,
) -> Result<(), AnyError> {
// ensure an "npm install" is done if the user has explicitly
Expand All @@ -454,13 +563,6 @@ impl ModuleGraphBuilder {
}
}
}
for (module, checksum) in &lockfile.content.remote {
if let Ok(module) = ModuleSpecifier::parse(module) {
graph
.checksums
.insert(module, LoaderChecksum::new(checksum.clone()));
}
}
for (key, value) in &lockfile.content.packages.specifiers {
if let Some(key) = key
.strip_prefix("jsr:")
Expand Down Expand Up @@ -492,7 +594,6 @@ impl ModuleGraphBuilder {

// todo: handle jsr package manifest checksums
let initial_redirects_len = graph.redirects.len();
let initial_checksums_len = graph.checksums.len();
let initial_packages_len = graph.packages.packages_len();
let initial_package_mappings_len = graph.packages.mappings().len();
let initial_npm_packages = graph.npm_packages.len();
Expand All @@ -511,7 +612,6 @@ impl ModuleGraphBuilder {
}

let has_redirects_changed = graph.redirects.len() != initial_redirects_len;
let has_checksums_changed = graph.checksums.len() != initial_checksums_len;
let has_jsr_packages_changed =
graph.packages.packages_len() != initial_packages_len;
let has_jsr_package_mappings_changed =
Expand All @@ -520,7 +620,6 @@ impl ModuleGraphBuilder {
graph.npm_packages.len() != initial_npm_packages;

if has_redirects_changed
|| has_checksums_changed
|| has_jsr_packages_changed
|| has_jsr_package_mappings_changed
|| has_npm_packages_changed
Expand All @@ -536,15 +635,6 @@ impl ModuleGraphBuilder {
lockfile.insert_redirect(from.to_string(), to.to_string());
}
}
// https module checksums
if has_checksums_changed {
for (module, checksum) in graph.checksums.iter() {
lockfile
.content
.remote
.insert(module.to_string(), checksum.as_str().to_string());
}
}
// jsr package mappings
if has_jsr_package_mappings_changed {
for (from, to) in graph.packages.mappings() {
Expand Down Expand Up @@ -703,21 +793,14 @@ pub fn enhanced_resolution_error_message(error: &ResolutionError) -> String {
message
}

pub fn enhanced_module_error_message(
fn sloppy_imports_error_message(
fs: Arc<dyn FileSystem>,
specifier: &ModuleSpecifier,
error: &ModuleError,
) -> String {
let additional_message = match error {
ModuleError::LoadingErr(specifier, _, _) // ex. "Is a directory" error
| ModuleError::Missing(specifier, _) => {
SloppyImportsResolver::new(fs).resolve(
specifier,
ResolutionMode::Execution,
)
.as_suggestion_message()
}
_ => None,
};
let additional_message = SloppyImportsResolver::new(fs)
.resolve(specifier, ResolutionMode::Execution)
.as_suggestion_message();
if let Some(message) = additional_message {
format!(
"{} {} or run with --unstable-sloppy-imports",
Expand Down
10 changes: 0 additions & 10 deletions cli/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -292,16 +292,6 @@ fn exit_for_error(error: AnyError) -> ! {
{
error_string = e.to_string();
error_code = 10;
} else if let Some(deno_graph::ModuleGraphError::ModuleError(
deno_graph::ModuleError::LoadingErr(_, _, e),
)) = error.downcast_ref::<deno_graph::ModuleGraphError>()
{
match e {
deno_graph::ModuleLoadError::HttpsChecksumIntegrity(_) => {
error_code = 10;
}
_ => {}
}
}

exit_with_message(&error_string, error_code);
Expand Down
2 changes: 1 addition & 1 deletion cli/tools/doc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,11 +61,11 @@ async fn generate_doc_nodes_for_builtin_types(
executor: Default::default(),
file_system: &NullFileSystem,
jsr_url_provider: Default::default(),
locker: None,
module_analyzer: analyzer,
npm_resolver: None,
reporter: None,
resolver: None,
verify_and_fill_checksums: false,
},
)
.await;
Expand Down
1 change: 0 additions & 1 deletion cli/tools/vendor/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,6 @@ impl VendorTestBuilder {
parsed_source_cache: &parsed_source_cache,
output_dir: &output_dir,
maybe_original_import_map: self.original_import_map.as_ref(),
maybe_lockfile: None,
maybe_jsx_import_source: self.jsx_import_source_config.as_ref(),
resolver: resolver.as_graph_resolver(),
environment: &self.environment,
Expand Down
7 changes: 0 additions & 7 deletions tests/integration/bundle_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -416,13 +416,6 @@ fn bundle_json_module_escape_sub() {
);
}

itest!(lockfile_check_error {
args: "bundle --lock=bundle/lockfile/check_error.json https://127.0.0.1:4545/subdir/mod1.ts",
output: "bundle/lockfile/check_error.out",
exit_code: 10,
http_server: true,
});

itest!(bundle {
args: "bundle subdir/mod1.ts",
output: "bundle/bundle.test.out",
Expand Down
Loading