Skip to content

Don't panic when dev files are not found during clean. #7622

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

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
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
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
#### :bug: Bug fix

- Fix `typeof` parens on functions. https://github.com/rescript-lang/rescript/pull/7643

- Don't panic when dev files are not present during clean. https://github.com/rescript-lang/rescript/pull/7622

# 12.0.0-beta.1

Expand Down
12 changes: 8 additions & 4 deletions rewatch/src/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@ pub mod packages;
pub mod parse;
pub mod read_compile_state;

use self::compile::compiler_args;
use self::parse::parser_args;
use crate::build::compile::{mark_modules_with_deleted_deps_dirty, mark_modules_with_expired_deps_dirty};
use crate::build::packages::DevDeps;
use crate::helpers::emojis::*;
use crate::helpers::{self, get_workspace_root};
use crate::sourcedirs;
Expand All @@ -26,9 +29,6 @@ use std::path::{Path, PathBuf};
use std::process::Stdio;
use std::time::{Duration, Instant};

use self::compile::compiler_args;
use self::parse::parser_args;

fn is_dirty(module: &Module) -> bool {
match module.source_type {
SourceType::SourceFile(SourceFile {
Expand Down Expand Up @@ -133,7 +133,11 @@ pub fn initialize_build(
&project_root,
&workspace_root,
show_progress,
build_dev_deps,
if build_dev_deps {
DevDeps::Build
} else {
DevDeps::DontBuild
},
)?;
let timing_package_tree_elapsed = timing_package_tree.elapsed();

Expand Down
3 changes: 2 additions & 1 deletion rewatch/src/build/clean.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use super::build_types::*;
use super::packages;
use crate::build::packages::DevDeps;
use crate::helpers;
use crate::helpers::emojis::*;
use ahash::AHashSet;
Expand Down Expand Up @@ -341,7 +342,7 @@ pub fn clean(path: &Path, show_progress: bool, snapshot_output: bool) -> Result<
show_progress,
// Build the package tree with dev dependencies.
// They should always be cleaned if they are there.
true,
DevDeps::Clean,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This used to be true, so dev files are included.
Yet, these files never existed in the first place because they were never shipped as part of npm package.

)?;
let root_config_name = packages::read_package_name(&project_root)?;
let bsc_path = helpers::get_bsc();
Expand Down
30 changes: 15 additions & 15 deletions rewatch/src/build/packages.rs
Original file line number Diff line number Diff line change
Expand Up @@ -501,7 +501,7 @@ pub fn get_source_files(
package_dir: &Path,
filter: &Option<regex::Regex>,
source: &config::PackageSource,
build_dev_deps: bool,
dev_deps: DevDeps,
) -> AHashMap<PathBuf, SourceFileMeta> {
let mut map: AHashMap<PathBuf, SourceFileMeta> = AHashMap::new();

Expand All @@ -515,8 +515,9 @@ pub fn get_source_files(
};

let path_dir = Path::new(&source.dir);
match (build_dev_deps, type_) {
(false, Some(type_)) if type_ == "dev" => (),
match (dev_deps, type_) {
(DevDeps::DontBuild, Some(type_)) if type_ == "dev" => (),
(DevDeps::Clean, Some(type_)) if type_ == "dev" && !package_dir.join(path_dir).exists() => (),
_ => match read_folders(filter, package_dir, path_dir, recurse) {
Ok(files) => map.extend(files),

Expand All @@ -537,22 +538,14 @@ pub fn get_source_files(
fn extend_with_children(
filter: &Option<regex::Regex>,
mut build: AHashMap<String, Package>,
build_dev_deps: bool,
dev_deps: DevDeps,
) -> AHashMap<String, Package> {
for (_key, package) in build.iter_mut() {
let mut map: AHashMap<PathBuf, SourceFileMeta> = AHashMap::new();
package
.source_folders
.par_iter()
.map(|source| {
get_source_files(
&package.name,
Path::new(&package.path),
filter,
source,
build_dev_deps,
)
})
.map(|source| get_source_files(&package.name, Path::new(&package.path), filter, source, dev_deps))
.collect::<Vec<AHashMap<PathBuf, SourceFileMeta>>>()
.into_iter()
.for_each(|source| map.extend(source));
Expand Down Expand Up @@ -582,6 +575,13 @@ fn extend_with_children(
build
}

#[derive(Clone, Copy)]
pub enum DevDeps {
Build,
DontBuild,
Clean,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this variant. We want to know if we need to build dev deps or not. Cleaning is a separate task, not a property of we need to build dev deps or not. Whether to clean can be derived from if we want to build or not (if we build we also need to clean). Or am I misunderstanding this?

}

/// Make turns a folder, that should contain a config, into a tree of Packages.
/// It does so in two steps:
/// 1. Get all the packages parsed, and take all the source folders from the config
Expand All @@ -594,13 +594,13 @@ pub fn make(
root_folder: &Path,
workspace_root: &Option<PathBuf>,
show_progress: bool,
build_dev_deps: bool,
dev_deps: DevDeps,
) -> Result<AHashMap<String, Package>> {
let map = read_packages(root_folder, workspace_root, show_progress)?;

/* Once we have the deduplicated packages, we can add the source files for each - to minimize
* the IO */
let result = extend_with_children(filter, map, build_dev_deps);
let result = extend_with_children(filter, map, dev_deps);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Cleaning and building share this code.


Ok(result)
}
Expand Down
6 changes: 4 additions & 2 deletions rewatch/testrepo/bsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@
"@testrepo/new-namespace",
"@testrepo/namespace-casing",
"@testrepo/with-dev-deps",
"@testrepo/compiled-by-legacy"
"@testrepo/compiled-by-legacy",
"@testrepo/nonexisting-dev-files"
],
"bs-dependencies": [
"@testrepo/main",
Expand All @@ -30,6 +31,7 @@
"@testrepo/new-namespace",
"@testrepo/namespace-casing",
"@testrepo/with-dev-deps",
"@testrepo/compiled-by-legacy"
"@testrepo/compiled-by-legacy",
"@testrepo/nonexisting-dev-files"
]
}
3 changes: 2 additions & 1 deletion rewatch/testrepo/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@
"packages/new-namespace",
"packages/namespace-casing",
"packages/with-dev-deps",
"packages/compiled-by-legacy"
"packages/compiled-by-legacy",
"packages/nonexisting-dev-files"
]
},
"dependencies": {
Expand Down
9 changes: 9 additions & 0 deletions rewatch/testrepo/packages/nonexisting-dev-files/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"name": "@testrepo/nonexisting-dev-files",
"version": "0.0.1",
"keywords": [
"rescript"
],
"author": "",
"license": "MIT"
}
8 changes: 8 additions & 0 deletions rewatch/testrepo/packages/nonexisting-dev-files/rescript.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"name": "@testrepo/nonexisting-dev-files",
"sources": {
"dir": "dev",
"subdirs": true,
"type": "dev"
}
}
6 changes: 6 additions & 0 deletions rewatch/testrepo/yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,12 @@ __metadata:
languageName: unknown
linkType: soft

"@testrepo/nonexisting-dev-files@workspace:packages/nonexisting-dev-files":
version: 0.0.0-use.local
resolution: "@testrepo/nonexisting-dev-files@workspace:packages/nonexisting-dev-files"
languageName: unknown
linkType: soft

"@testrepo/with-dev-deps@workspace:packages/with-dev-deps":
version: 0.0.0-use.local
resolution: "@testrepo/with-dev-deps@workspace:packages/with-dev-deps"
Expand Down
Loading