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.

Sign up for GitHub

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

Jump to bottom

fix: clean paths and use package path instead of package.json path #6649

Merged
merged 6 commits into from
Dec 1, 2023
Merged

fix: clean paths and use package path instead of package.json path #6649

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3e9abff
Add failing pkg inference + filter integration test
Nov 30, 2023
fcb0f7b
fix: clean paths and use package path instead of package.json path
Nov 30, 2023
4b4362c
chore: add failing filter tests
Nov 30, 2023
665bdcf
Merge branch 'olszewski/failing_filter_tests' into olszewski/fix_targ…
Nov 30, 2023
07edae0
fix: correctly clean package inference paths
Nov 30, 2023
5b10a64
fix: update test to now match correct behavior
Nov 30, 2023
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
1 change: 1 addition & 0 deletions Cargo.lock
View file
Open in desktop

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

8 changes: 8 additions & 0 deletions cli/internal/scope/filter/filter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,14 @@ func Test_filter(t *testing.T) {
nil,
[]string{"project-0", "project-1"},
},
{
"select sibling directory",
[]*TargetSelector{{parentDir: turbopath.MakeRelativeSystemPath("..", "packages", "*")}},
&PackageInference{
DirectoryRoot: turbopath.MakeRelativeSystemPath("project-5"),
},
[]string{"project-0", "project-1"},
},
{
"select by parentDir using globstar",
[]*TargetSelector{
Expand Down
13 changes: 13 additions & 0 deletions cli/internal/scope/filter/parse_target_selector_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,19 @@ func TestParseTargetSelector(t *testing.T) {
},
false,
},
{
"./foo/*",
&TargetSelector{
fromRef: "",
exclude: false,
excludeSelf: false,
includeDependencies: false,
includeDependents: false,
namePattern: "",
parentDir: turbopath.MakeRelativeSystemPath("foo/*"),
},
false,
},
{
"../foo",
&TargetSelector{
Expand Down
1 change: 1 addition & 0 deletions crates/turborepo-lib/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ jsonc-parser = { version = "0.21.0" }
lazy_static = { workspace = true }
libc = "0.2.140"
notify = "5.1"
path-clean = "1.0.1"
Copy link
Member Author

Choose a reason for hiding this comment

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

This library was intentionally written to match the behavior of Go's filepath.Clean. I did a spot check and it does seem to be a true to form port, but more eyes are always welcome.
filepath.Clean
path_clean::clean

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, we already use it in turbopath. Might be worth making a root dependency?

petgraph = { workspace = true }
pidlock = { path = "../turborepo-pidlock" }
prost = "0.11.6"
Expand Down
40 changes: 31 additions & 9 deletions crates/turborepo-lib/src/run/scope/filter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ impl PackageInference {
);
let full_inference_path = turbo_root.resolve(pkg_inference_path);
for (workspace_name, workspace_entry) in pkg_graph.workspaces() {
let pkg_path = turbo_root.resolve(workspace_entry.package_json_path());
let pkg_path = turbo_root.resolve(workspace_entry.package_path());
Copy link
Member Author

Choose a reason for hiding this comment

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

Previously we would be checking if /path/to/repo/packages/foo/package.json contained a directory which would always be false.

let inferred_path_is_below = pkg_path.contains(&full_inference_path);
// We skip over the root package as the inferred path will always be below it
if inferred_path_is_below && (&pkg_path as &AbsoluteSystemPath) != turbo_root {
Expand All @@ -47,7 +47,7 @@ impl PackageInference {
// do so in a consistent manner
return Self {
package_name: Some(workspace_name.to_string()),
directory_root: workspace_entry.package_json_path().to_owned(),
directory_root: workspace_entry.package_path().to_owned(),
};
}
let inferred_path_is_between_root_and_pkg = full_inference_path.contains(&pkg_path);
Expand Down Expand Up @@ -75,7 +75,14 @@ impl PackageInference {
}

if selector.parent_dir != turbopath::AnchoredSystemPathBuf::default() {
selector.parent_dir = self.directory_root.join(&selector.parent_dir);
let repo_relative_parent_dir = self.directory_root.join(&selector.parent_dir);
let clean_parent_dir =
Copy link
Contributor

Choose a reason for hiding this comment

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

Definitely not blocking, but we ought to think about how to work this into the path library properly

path_clean::clean(std::path::Path::new(repo_relative_parent_dir.as_path()))
.into_os_string()
.into_string()
.expect("path was valid utf8 before cleaning");
selector.parent_dir = AnchoredSystemPathBuf::try_from(clean_parent_dir.as_str())
.expect("path wasn't absolute before cleaning");
} else if self.package_name.is_none() {
// fallback: the user didn't set a parent directory and we didn't find a single
// package, so use the directory we inferred and select all subdirectories
Expand Down Expand Up @@ -343,7 +350,7 @@ impl<'a, T: PackageChangeDetector> FilterResolver<'a, T> {
} else {
let path = selector.parent_dir.to_unix();
let parent_dir_matcher = wax::Glob::new(path.as_str())?;
let matches = parent_dir_matcher.is_match(info.package_json_path.as_path());
let matches = parent_dir_matcher.is_match(info.package_path().as_path());

if matches {
entry_packages.insert(name.to_owned());
Expand Down Expand Up @@ -413,7 +420,7 @@ impl<'a, T: PackageChangeDetector> FilterResolver<'a, T> {
let package_path_lookup = self
.pkg_graph
.workspaces()
.map(|(name, entry)| (name, entry.package_json_path()))
.map(|(name, entry)| (name, entry.package_path()))
.collect::<HashMap<_, _>>();

for package in changed_packages {
Expand Down Expand Up @@ -446,7 +453,7 @@ impl<'a, T: PackageChangeDetector> FilterResolver<'a, T> {
} else {
let packages = self.pkg_graph.workspaces();
for (name, _) in packages.filter(|(_name, info)| {
let path = info.package_json_path.as_path();
let path = info.package_path().as_path();
parent_dir_globber.is_match(path)
}) {
entry_packages.insert(name.to_owned());
Expand Down Expand Up @@ -625,7 +632,7 @@ mod test {
AbsoluteSystemPathBuf::new(temp_folder.path().as_os_str().to_str().unwrap()).unwrap(),
));

let packages = dependencies
let package_dirs = dependencies
.iter()
.flat_map(|(a, b)| vec![a, b])
.chain(extras.iter())
Expand All @@ -641,13 +648,16 @@ mod test {
acc
});

let package_jsons = packages
let package_jsons = package_dirs
.iter()
.map(|package_path| {
let (_, name) = get_name(package_path);
(
turbo_root
.join_unix_path(RelativeUnixPathBuf::new(**package_path).unwrap())
.join_unix_path(
RelativeUnixPathBuf::new(format!("{package_path}/package.json"))
Copy link
Member Author

Choose a reason for hiding this comment

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

The keys to the package json map for the package graph builder should be paths to package.json files as this is what will get returned by a the PackageManager::get_package_jsons.

This caused the test fixture to cover up the logic errors in the implementation.

.unwrap(),
)
.unwrap(),
PackageJson {
name: Some(name.to_string()),
Expand Down Expand Up @@ -808,6 +818,18 @@ mod test {
&["project-0", "project-1"] ;
"select by parentDir using glob"
)]
#[test_case(
vec![TargetSelector {
parent_dir: AnchoredSystemPathBuf::try_from(if cfg!(windows) { "..\\packages\\*" } else { "../packages/*" }).unwrap(),
..Default::default()
}],
Some(PackageInference{
package_name: None,
directory_root: AnchoredSystemPathBuf::try_from("project-5").unwrap(),
}),
&["project-0", "project-1"] ;
"select sibling directory"
)]
#[test_case(
vec![
TargetSelector {
Expand Down
19 changes: 14 additions & 5 deletions crates/turborepo-lib/src/run/scope/target_selector.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,11 @@ impl FromStr for TargetSelector {
if directory.is_empty() {
return Err(InvalidSelectorError::EmptyPathSpecification);
} else {
parent_dir = AnchoredSystemPathBuf::try_from(directory.as_str())
let clean_directory = path_clean::clean(std::path::Path::new(directory.as_str()))
.into_os_string()
.into_string()
.expect("directory was valid utf8 before cleaning");
parent_dir = AnchoredSystemPathBuf::try_from(clean_directory.as_str())
.map_err(|_| InvalidSelectorError::InvalidAnchoredPath(directory))?;
}
}
Expand Down Expand Up @@ -195,8 +199,12 @@ pub fn is_selector_by_location(
.iter()
.any(|prefix| raw_selector.starts_with(prefix))
{
let cleaned_selector = path_clean::clean(std::path::Path::new(raw_selector))
Copy link
Member Author

Choose a reason for hiding this comment

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

We do this to match the Go behavior where turbopath.MakeRelativeSystemPath called filepath.Clean under the hood which would remove any leading ./.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should clean all AnchoredSystemPathBuf and RelativeUnixPaths such that there are as few ways to represent the same path as possible.

.into_os_string()
.into_string()
Comment on lines +203 to +204
Copy link
Member Author

Choose a reason for hiding this comment

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

We could use AnchoredSystemPathBuf::from_system_path to avoid these conversion, but that codepath will failif the path starts with ../ which is a valid filter.

.expect("raw selector was valid utf8");
Some(
AnchoredSystemPathBuf::try_from(raw_selector)
AnchoredSystemPathBuf::try_from(cleaned_selector.as_str())
.map_err(|_| InvalidSelectorError::InvalidAnchoredPath(raw_selector.to_string())),
)
} else {
Expand All @@ -219,9 +227,10 @@ mod test {
#[test_case("...foo...", TargetSelector { name_pattern: "foo".to_string(), raw: "...foo...".to_string(), include_dependents: true, include_dependencies: true, ..Default::default() }; "dot dot dot foo dot dot dot")]
#[test_case("foo^...", TargetSelector { name_pattern: "foo".to_string(), raw: "foo^...".to_string(), include_dependencies: true, exclude_self: true, ..Default::default() }; "foo caret dot dot dot")]
#[test_case("...^foo", TargetSelector { name_pattern: "foo".to_string(), raw: "...^foo".to_string(), include_dependents: true, exclude_self: true, ..Default::default() }; "dot dot dot caret foo")]
#[test_case("./foo", TargetSelector { raw: "./foo".to_string(), parent_dir: AnchoredSystemPathBuf::try_from("./foo").unwrap(), ..Default::default() }; "dot slash foo")]
#[test_case("../foo", TargetSelector { raw: "../foo".to_string(), parent_dir: AnchoredSystemPathBuf::try_from("../foo").unwrap(), ..Default::default() }; "dot dot slash foo")]
#[test_case("...{./foo}", TargetSelector { raw: "...{./foo}".to_string(), parent_dir: AnchoredSystemPathBuf::try_from("./foo").unwrap(), include_dependents: true, ..Default::default() }; "dot dot dot curly bracket foo")]
#[test_case("../foo", TargetSelector { raw: "../foo".to_string(), parent_dir: AnchoredSystemPathBuf::try_from(if cfg!(windows) { "..\\foo" } else { "../foo" }).unwrap(), ..Default::default() }; "dot dot slash foo")]
#[test_case("./foo", TargetSelector { raw: "./foo".to_string(), parent_dir: AnchoredSystemPathBuf::try_from("foo").unwrap(), ..Default::default() }; "dot slash foo")]
Copy link
Member Author

Choose a reason for hiding this comment

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

parent_dir: "foo" now matches the expected value of the corresponding Go unit test

#[test_case("./foo/*", TargetSelector { raw: "./foo/*".to_string(), parent_dir: AnchoredSystemPathBuf::try_from(if cfg!(windows) { "foo\\*" } else { "foo/*" }).unwrap(), ..Default::default() }; "dot slash foo star")]
#[test_case("...{./foo}", TargetSelector { raw: "...{./foo}".to_string(), parent_dir: AnchoredSystemPathBuf::try_from("foo").unwrap(), include_dependents: true, ..Default::default() }; "dot dot dot curly bracket foo")]
#[test_case(".", TargetSelector { raw: ".".to_string(), parent_dir: AnchoredSystemPathBuf::try_from(".").unwrap(), ..Default::default() }; "parent dir dot")]
#[test_case("..", TargetSelector { raw: "..".to_string(), parent_dir: AnchoredSystemPathBuf::try_from("..").unwrap(), ..Default::default() }; "parent dir dot dot")]
#[test_case("[master]", TargetSelector { raw: "[master]".to_string(), from_ref: "master".to_string(), ..Default::default() }; "square brackets master")]
Expand Down
45 changes: 44 additions & 1 deletion turborepo-tests/integration/tests/run/infer-pkg.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,39 @@ Run a dry run
"util"
]

Run a dry run in packages with a glob filter
$ ${TURBO} build --dry=json -F "./packages/*" | jq .packages
[
"another",
"util"
]

Run a dry run in packages with a name glob
$ ${TURBO} build --dry=json -F "*-app" | jq .packages
[
"my-app"
]

Run a dry run in packages with a filter
$ cd packages
$ ${TURBO} build --dry=json -F "{./util}" | jq .packages
[
"util"
]
Run a dry run with a filter from a sibling directory
$ ${TURBO} build --dry=json -F "../apps/*" | jq .packages
[
"my-app"
]

Run a dry run with a filter name glob
$ ${TURBO} build --dry=json -F "*-app" | jq .packages
[
"my-app"
]

Run a dry run in a directory
$ cd packages/util
$ cd util
$ ${TURBO} build --dry=json | jq .packages
[
"util"
Expand All @@ -24,3 +55,15 @@ Ensure we don't infer packages if --cwd is supplied
"my-app",
"util"
]

Run a dry run in packages with a glob filter from directory
$ ${TURBO} build --dry=json -F "../*" | jq .packages
[
"util"
]

Run a dry run in packages with a name glob from directory
$ ${TURBO} build --dry=json -F "*nother" | jq .packages
[
"another"
]
Loading