Skip to content
Draft
Changes from all commits
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
162 changes: 149 additions & 13 deletions src/plugins/core/node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -723,25 +723,62 @@ impl Backend for NodePlugin {
fn resolve_lockfile_options(
&self,
_request: &ToolRequest,
target: &PlatformTarget,
_target: &PlatformTarget,
) -> BTreeMap<String, String> {
let mut opts = BTreeMap::new();
let settings = Settings::get();
let is_current_platform = target.is_current();
let node = &settings.node;

// Only include compile option if true (non-default)
let compile = if is_current_platform {
settings.node.compile.unwrap_or(false)
} else {
false
opts.insert("mirror_url".to_string(), node.mirror_url().to_string());

let compile = match node.compile {
Some(true) => "true",
Some(false) => "false",
None => "auto",
};
if compile {
opts.insert("compile".to_string(), "true".to_string());
opts.insert("compile".to_string(), compile.to_string());

if node.compile != Some(false) {
if let Some(cflags) = node.cflags().filter(|cflags| !cflags.is_empty()) {
opts.insert("cflags".to_string(), cflags);
}
if let Some(configure_opts) = node
.configure_opts
.clone()
.or_else(|| env::var("NODE_CONFIGURE_OPTS").ok())
.filter(|configure_opts| !configure_opts.is_empty())
{
opts.insert("configure_opts".to_string(), configure_opts);
}
let make = node.make.clone().unwrap_or_else(|| "make".into());
if !make.is_empty() {
opts.insert("make".to_string(), make);
}
if let Some(make_opts) = node
.make_opts
.clone()
.or_else(|| env::var("NODE_MAKE_OPTS").ok())
.filter(|make_opts| !make_opts.is_empty())
{
opts.insert("make_opts".to_string(), make_opts);
}
if let Some(make_install_opts) = node
.make_install_opts
.clone()
.or_else(|| env::var("NODE_MAKE_INSTALL_OPTS").ok())
.filter(|make_install_opts| !make_install_opts.is_empty())
{
opts.insert("make_install_opts".to_string(), make_install_opts);
}
opts.insert("ninja".to_string(), node.ninja().to_string());
if let Some(concurrency) = node.concurrency() {
opts.insert("concurrency".to_string(), concurrency.to_string());
}
Comment on lines +774 to +776
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Machine-specific concurrency embedded in lock identity

node.concurrency() falls back to num_cpus::get_physical() when the user has not set node.concurrency and ninja is not in use (see SettingsNode::concurrency()). That CPU-count value is machine-dependent, so two developers with different core counts who share a lockfile compute different option sets and never match each other's lock entries, even though the resulting Node binary is identical regardless of -j parallelism. Consider using the raw node.concurrency field directly here so only an explicitly-configured value enters the lock identity — when it is None, simply omit the key.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

}

// Flavor affects which binary variant is downloaded
// Apply to all platforms to avoid splitting lockfile entries (#8390)
if let Some(flavor) = settings.node.flavor.clone() {
// Flavor affects which binary variant is downloaded.
// Apply to all platforms to avoid splitting lockfile entries (#8390).
if let Some(flavor) = node.flavor.clone().filter(|flavor| !flavor.is_empty()) {
opts.insert("flavor".to_string(), flavor);
}

Expand Down Expand Up @@ -946,7 +983,36 @@ struct NodeVersion {
#[cfg(test)]
mod tests {
use super::*;
use crate::config::settings::SettingsNode;
use crate::config::settings::{SettingsNode, SettingsPartial};
use crate::toolset::ToolSource;
use confique::Layer;

static TEST_SETTINGS_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(());

struct SettingsResetGuard {
_lock: std::sync::MutexGuard<'static, ()>,
}

impl Drop for SettingsResetGuard {
fn drop(&mut self) {
Settings::reset(None);
}
}

fn resolve_node_lockfile_options(
configure_settings: impl FnOnce(&mut SettingsPartial),
) -> BTreeMap<String, String> {
let lock = TEST_SETTINGS_LOCK.lock().unwrap();
let mut settings = SettingsPartial::empty();
configure_settings(&mut settings);
Settings::reset(Some(settings));
let _guard = SettingsResetGuard { _lock: lock };

let backend = NodePlugin::new();
let request = ToolRequest::new(backend.ba().clone(), "22.0.0", ToolSource::Unknown)
.expect("valid node request");
backend.resolve_lockfile_options(&request, &PlatformTarget::from_current())
}

#[test]
fn test_mirror_url_for_routes_musl_to_unofficial_builds() {
Expand Down Expand Up @@ -975,4 +1041,74 @@ mod tests {
assert_eq!(glibc.as_str(), "https://corp.example/node/");
assert_eq!(musl.as_str(), "https://corp.example/node/");
}

#[test]
fn test_node_lockfile_options_include_default_settings() {
let opts = resolve_node_lockfile_options(|_| {});

assert_eq!(
opts.get("mirror_url").map(String::as_str),
Some(DEFAULT_NODE_MIRROR_URL)
);
assert_eq!(opts.get("compile").map(String::as_str), Some("auto"));
assert_eq!(opts.get("make").map(String::as_str), Some("make"));
assert!(opts.contains_key("ninja"));
if opts.get("ninja").map(String::as_str) == Some("false") {
assert!(opts.contains_key("concurrency"));
}
}

#[test]
fn test_node_lockfile_options_include_binary_settings() {
let opts = resolve_node_lockfile_options(|settings| {
settings.node.compile = Some(false);
settings.node.mirror_url = Some("https://corp.example/node/".to_string());
settings.node.flavor = Some("musl".to_string());
});

assert_eq!(
opts,
BTreeMap::from([
("compile".to_string(), "false".to_string()),
("flavor".to_string(), "musl".to_string()),
(
"mirror_url".to_string(),
"https://corp.example/node/".to_string()
),
])
);
}

#[test]
fn test_node_lockfile_options_include_source_build_settings() {
let opts = resolve_node_lockfile_options(|settings| {
settings.node.compile = Some(true);
settings.node.mirror_url = Some(DEFAULT_NODE_MIRROR_URL.to_string());
settings.node.cflags = Some("-O2".to_string());
settings.node.configure_opts = Some("--openssl-no-asm".to_string());
settings.node.make = Some("gmake".to_string());
settings.node.make_opts = Some("-s".to_string());
settings.node.make_install_opts = Some("--no-strip".to_string());
settings.node.concurrency = Some(16);
settings.node.ninja = Some(false);
});

assert_eq!(
opts,
BTreeMap::from([
("cflags".to_string(), "-O2".to_string()),
("compile".to_string(), "true".to_string()),
("concurrency".to_string(), "16".to_string()),
("configure_opts".to_string(), "--openssl-no-asm".to_string()),
("make".to_string(), "gmake".to_string()),
("make_install_opts".to_string(), "--no-strip".to_string()),
("make_opts".to_string(), "-s".to_string()),
(
"mirror_url".to_string(),
DEFAULT_NODE_MIRROR_URL.to_string()
),
("ninja".to_string(), "false".to_string()),
])
);
}
}
Loading