Skip to content

Commit f285c19

Browse files
janherlingmetameta-codesync[bot]
authored andcommitted
Stop --dry-run from touching the filesystem
Summary: `--dry-run` is documented as "Show build plan without building". It did two things to disk before showing anything. **`--clean` ran first.** `main()` handles `--clean` at the point where the directory manager is created, which is above the `--dry-run` early return. So `--dry-run --clean` — a natural thing to type when you want to see what a clean rebuild would do — called `DirectoryManager.clean_all()` and `rmtree`d every fetched source and every build tree, then printed the plan and exited without rebuilding any of it. The next real build re-clones all 23 repositories. **The directory manager created its tree in `__init__`.** Constructing it mkdir'd `install/`, `source/` and `build/`, so a dry run in any directory left an `ocean_3rdparty/` skeleton behind — including when the plan it printed was for a `--output-dir` the user was only evaluating. `--clean` is now skipped under `--dry-run` with a line saying what it would have deleted, and `DirectoryManager` takes `create=False` so it resolves paths without creating them. Found by an exhaustive review of `build/python/`; tracked as `OB-038`. ___ Differential Revision: D116053695 fbshipit-source-id: 498aba375d891af9b1d312577c25c3e990bedc3f
1 parent a8042c8 commit f285c19

2 files changed

Lines changed: 17 additions & 6 deletions

File tree

build/python/build_ocean_3rdparty.py

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1899,16 +1899,22 @@ def main() -> int: # noqa: C901
18991899
source_dir,
19001900
build_dir,
19011901
for_external_integration=args.for_external_integration,
1902+
create=not args.dry_run,
19021903
)
19031904
except OSError as e:
19041905
print(f"Error: cannot create the output directories under {base_dir}: {e}")
19051906
return 1
19061907
fetcher = SourceFetcher(dir_manager, manifest_dir=manifest_path.parent)
19071908

1908-
# Handle --clean
1909+
# Handle --clean. Deliberately not done under --dry-run: deleting every
1910+
# fetched source and build tree is the most destructive thing this script
1911+
# can do, and "show me what you would build" must not do it.
19091912
if args.clean:
1910-
print("Cleaning cache...")
1911-
dir_manager.clean_all()
1913+
if args.dry_run:
1914+
print("Dry run: skipping --clean (would delete the source and build cache)")
1915+
else:
1916+
print("Cleaning cache...")
1917+
dir_manager.clean_all()
19121918

19131919
# Determine targets
19141920
if requested_platforms is not None:

build/python/lib/directories.py

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,7 @@ def __init__(
111111
source_dir: Optional[Path] = None,
112112
build_dir: Optional[Path] = None,
113113
for_external_integration: bool = False,
114+
create: bool = True,
114115
):
115116
"""Initialize directory manager.
116117
@@ -125,6 +126,9 @@ def __init__(
125126
(default), produce the standard CMake install layout per
126127
target (one complete prefix per library per target), which
127128
works directly with find_package(... CONFIG).
129+
create: If False, resolve the paths but do not create anything on
130+
disk. Used by --dry-run, which must not leave directories
131+
behind for a build it did not run.
128132
"""
129133
self.install_dir = install_dir.resolve()
130134
self.for_external_integration = for_external_integration
@@ -137,9 +141,10 @@ def __init__(
137141
self.builds_dir = (build_dir or default_cache / "builds").resolve()
138142

139143
# Create base directories
140-
self.sources_dir.mkdir(parents=True, exist_ok=True)
141-
self.builds_dir.mkdir(parents=True, exist_ok=True)
142-
self.install_dir.mkdir(parents=True, exist_ok=True)
144+
if create:
145+
self.sources_dir.mkdir(parents=True, exist_ok=True)
146+
self.builds_dir.mkdir(parents=True, exist_ok=True)
147+
self.install_dir.mkdir(parents=True, exist_ok=True)
143148

144149
def get_source_dir(self, library: str, version: str) -> Path:
145150
"""Get the source directory for a library (shared across all targets)."""

0 commit comments

Comments
 (0)