Skip to content

Commit

Permalink
fix(yarn): no longer error on pnp (#5009)
Browse files Browse the repository at this point in the history
### Description

Fixes #4953

We had unintentionally dropped the PnP check in Go and reintroduced the
check in Rust. This PR simply removes this check and returns us to the
old unintentional behavior. I believe run should work as expected since
we execute all tasks via `yarn` which automatically does the right thing
with `.pnp.cjs`. (Also #4953 contains many reports of people using Yarn
PnP with turbo just fine)

This PR does not:
- Add support for PnP with prune. That check was never accidentally
dropped and I want to do a few more tests before enabling that.
- Fixup our examples to work correctly with Yarn PnP. All of the ones I
tried required adding some dependencies

Reviewer notes:
I apologize, but I ended up fixing all the clippy lints in the files I
touched and not just the ones introduced from the behavior change. This
resulted in the PR being larger than necessary.

### Testing Instructions

Existing unit tests around yarn/berry detection now pass without the a
`.yarnrc.yml` specifying `nodeLinker: node-modules`. Manual testing of
some test repos

Co-authored-by: Chris Olszewski <Chris Olszewski>
  • Loading branch information
chris-olszewski committed May 18, 2023
1 parent f89354f commit 77277af
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 65 deletions.
39 changes: 14 additions & 25 deletions crates/turborepo-lib/src/package_manager/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ use anyhow::{anyhow, Result};
use itertools::Itertools;
use regex::Regex;
use serde::{Deserialize, Serialize};
use turbopath::AbsoluteSystemPathBuf;

use crate::{
commands::CommandBase,
Expand Down Expand Up @@ -166,9 +165,7 @@ impl PackageManager {
// We don't surface errors for `read_package_manager` as we can fall back to
// `detect_package_manager`
if let Some(package_json) = pkg {
if let Ok(Some(package_manager)) =
Self::read_package_manager(&base.repo_root, package_json)
{
if let Ok(Some(package_manager)) = Self::read_package_manager(package_json) {
return Ok(package_manager);
}
}
Expand All @@ -177,10 +174,7 @@ impl PackageManager {
}

// Attempts to read the package manager from the package.json
fn read_package_manager(
repo_root: &AbsoluteSystemPathBuf,
pkg: &PackageJson,
) -> Result<Option<Self>> {
fn read_package_manager(pkg: &PackageJson) -> Result<Option<Self>> {
let Some(package_manager) = &pkg.package_manager else {
return Ok(None)
};
Expand All @@ -189,7 +183,7 @@ impl PackageManager {
let version = version.parse()?;
let manager = match manager {
"npm" => Some(PackageManager::Npm),
"yarn" => Some(YarnDetector::detect_berry_or_yarn(repo_root, &version)?),
"yarn" => Some(YarnDetector::detect_berry_or_yarn(&version)?),
"pnpm" => Some(PnpmDetector::detect_pnpm6_or_pnpm(&version)?),
_ => None,
};
Expand Down Expand Up @@ -246,9 +240,10 @@ mod tests {
use std::{fs::File, path::Path};

use tempfile::tempdir;
use turbopath::AbsoluteSystemPathBuf;

use super::*;
use crate::{get_version, package_manager::yarn::YARN_RC, ui::UI, Args};
use crate::{get_version, ui::UI, Args};

struct TestCase {
name: String,
Expand Down Expand Up @@ -340,32 +335,26 @@ mod tests {

#[test]
fn test_read_package_manager() -> Result<()> {
let repo_root = tempdir()?;
let mut package_json = PackageJson::default();
let repo_root_path = AbsoluteSystemPathBuf::new(repo_root.path())?;

// Set up .yarnrc.yml file
let yarn_rc_path = repo_root.path().join(YARN_RC);
fs::write(&yarn_rc_path, "nodeLinker: node-modules")?;

package_json.package_manager = Some("npm@8.19.4".to_string());
let package_manager = PackageManager::read_package_manager(&repo_root_path, &package_json)?;
let mut package_json = PackageJson {
package_manager: Some("npm@8.19.4".to_string()),
};
let package_manager = PackageManager::read_package_manager(&package_json)?;
assert_eq!(package_manager, Some(PackageManager::Npm));

package_json.package_manager = Some("yarn@2.0.0".to_string());
let package_manager = PackageManager::read_package_manager(&repo_root_path, &package_json)?;
let package_manager = PackageManager::read_package_manager(&package_json)?;
assert_eq!(package_manager, Some(PackageManager::Berry));

package_json.package_manager = Some("yarn@1.9.0".to_string());
let package_manager = PackageManager::read_package_manager(&repo_root_path, &package_json)?;
let package_manager = PackageManager::read_package_manager(&package_json)?;
assert_eq!(package_manager, Some(PackageManager::Yarn));

package_json.package_manager = Some("pnpm@6.0.0".to_string());
let package_manager = PackageManager::read_package_manager(&repo_root_path, &package_json)?;
let package_manager = PackageManager::read_package_manager(&package_json)?;
assert_eq!(package_manager, Some(PackageManager::Pnpm6));

package_json.package_manager = Some("pnpm@7.2.0".to_string());
let package_manager = PackageManager::read_package_manager(&repo_root_path, &package_json)?;
let package_manager = PackageManager::read_package_manager(&package_json)?;
assert_eq!(package_manager, Some(PackageManager::Pnpm));

Ok(())
Expand All @@ -385,7 +374,7 @@ mod tests {
let package_lock_json_path = repo_root.path().join(npm::LOCKFILE);
File::create(&package_lock_json_path)?;
let pnpm_lock_path = repo_root.path().join(pnpm::LOCKFILE);
File::create(&pnpm_lock_path)?;
File::create(pnpm_lock_path)?;

let error = PackageManager::detect_package_manager(&base).unwrap_err();
assert_eq!(
Expand Down
47 changes: 7 additions & 40 deletions crates/turborepo-lib/src/package_manager/yarn.rs
Original file line number Diff line number Diff line change
@@ -1,21 +1,13 @@
use std::{fs::File, process::Command};
use std::process::Command;

use anyhow::{anyhow, Context, Result};
use anyhow::Result;
use node_semver::{Range, Version};
use serde::Deserialize;
use turbopath::{AbsoluteSystemPathBuf, RelativeSystemPathBuf};
use which::which;

use crate::package_manager::PackageManager;

#[derive(Debug, Deserialize)]
struct YarnRC {
#[serde(rename = "nodeLinker")]
node_linker: Option<String>,
}

pub const LOCKFILE: &str = "yarn.lock";
pub const YARN_RC: &str = ".yarnrc.yml";

pub struct YarnDetector<'a> {
repo_root: &'a AbsoluteSystemPathBuf,
Expand Down Expand Up @@ -46,34 +38,15 @@ impl<'a> YarnDetector<'a> {
let yarn_binary = which("yarn")?;
let output = Command::new(yarn_binary)
.arg("--version")
.current_dir(&self.repo_root)
.current_dir(self.repo_root)
.output()?;
let yarn_version_output = String::from_utf8(output.stdout)?;
Ok(yarn_version_output.trim().parse()?)
}

fn is_nm_linker(repo_root: &AbsoluteSystemPathBuf) -> Result<bool> {
let yarnrc_path = repo_root.join_relative(RelativeSystemPathBuf::new(YARN_RC)?);
let yarnrc = File::open(yarnrc_path)?;
let yarnrc: YarnRC = serde_yaml::from_reader(&yarnrc)?;
Ok(yarnrc.node_linker.as_deref() == Some("node-modules"))
}

pub fn detect_berry_or_yarn(
repo_root: &AbsoluteSystemPathBuf,
version: &Version,
) -> Result<PackageManager> {
pub fn detect_berry_or_yarn(version: &Version) -> Result<PackageManager> {
let berry_constraint: Range = ">=2.0.0-0".parse()?;
if berry_constraint.satisfies(version) {
let is_nm_linker = Self::is_nm_linker(repo_root)
.context("could not determine if yarn is using `nodeLinker: node-modules`")?;

if !is_nm_linker {
return Err(anyhow!(
"only yarn v2/v3 with `nodeLinker: node-modules` is supported at this time"
));
}

Ok(PackageManager::Berry)
} else {
Ok(PackageManager::Yarn)
Expand All @@ -97,7 +70,7 @@ impl<'a> Iterator for YarnDetector<'a> {
if yarn_lockfile.exists() {
Some(
self.get_yarn_version()
.and_then(|version| Self::detect_berry_or_yarn(self.repo_root, &version)),
.and_then(|version| Self::detect_berry_or_yarn(&version)),
)
} else {
None
Expand All @@ -107,7 +80,7 @@ impl<'a> Iterator for YarnDetector<'a> {

#[cfg(test)]
mod tests {
use std::{fs, fs::File};
use std::fs::File;

use anyhow::Result;
use tempfile::tempdir;
Expand All @@ -117,10 +90,7 @@ mod tests {
use crate::{
commands::CommandBase,
get_version,
package_manager::{
yarn::{YarnDetector, YARN_RC},
PackageManager,
},
package_manager::{yarn::YarnDetector, PackageManager},
ui::UI,
Args,
};
Expand All @@ -139,9 +109,6 @@ mod tests {
let yarn_lock_path = repo_root.path().join(LOCKFILE);
File::create(&yarn_lock_path)?;

let yarn_rc_path = repo_root.path().join(YARN_RC);
fs::write(&yarn_rc_path, "nodeLinker: node-modules")?;

let absolute_repo_root = AbsoluteSystemPathBuf::new(base.repo_root)?;
let mut detector = YarnDetector::new(&absolute_repo_root);
detector.set_version_override("1.22.10".parse()?);
Expand Down

0 comments on commit 77277af

Please sign in to comment.