Skip to content

Commit

Permalink
Remove existing entry if any when materialising symlink (#18873)
Browse files Browse the repository at this point in the history
This applies a workaround that fixes #18809, for 2.16: before this PR,
repeated commands that write the exact same contents to `dist/` will
fail, if those contents include a symlink. After this patch, they will
succeed. For instance, `pants export-codegen ::` twice if any codegen
creates a symlink.

The particular problem of failing when re-materialising an entry only
surfaces with symlinks, because directories are created in "exists okay"
mode, and files are truncated if they already exist.

However, directories and files _do_ have problems when being
materialised over an entry of a different kind (#17758), but fixing that
seems like a broader issue, and likely too large to target 2.16 at this
point. After the change in this PR, we're at least back to the behaviour
in 2.15:

- directly rerunning commands that write to the workspace will always
succeed
- rerunning after changes may or may not (and, if it does, may or may
not give a valid result: #18849)

I've started on a potential fix for #17758 and #18849 in #18871, but, as
mentioned, it felt like it was getting too large and too "feature"-y to
land for 2.16. If/when a fix along those lines lands, this workaround
can likely be reverted.
  • Loading branch information
huonw authored May 1, 2023
1 parent 67b532e commit 07dbcdb
Showing 1 changed file with 22 additions and 6 deletions.
28 changes: 22 additions & 6 deletions src/rust/engine/fs/store/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1442,12 +1442,28 @@ impl Store {
destination: PathBuf,
target: String,
) -> Result<(), StoreError> {
symlink(&target, &destination).await.map_err(|e| {
format!(
"Failed to create symlink to {target} at {}: {e}",
destination.display()
)
})?;
// Overwriting a symlink, even with another symlink, fails if it exists. This can occur when
// materializing to a fixed directory like dist/. To avoid pessimising the more common case (no
// overwrite, e.g. materializing to a temp dir), only remove after noticing a failure.
//
// NB. #17758, #18849: this is a work-around for inaccurate management of the contents of dist/.
for first in [true, false] {
match symlink(&target, &destination).await {
Ok(()) => break,
Err(e) if first && e.kind() == std::io::ErrorKind::AlreadyExists => {
tokio::fs::remove_dir_all(&destination).await.map_err(|e| {
format!(
"Failed to remove existing item at {} when creating symlink to {target} there: {e}",
destination.display()
)
})?
}
Err(e) => Err(format!(
"Failed to create symlink to {target} at {}: {e}",
destination.display()
))?,
}
}
Ok(())
}

Expand Down

0 comments on commit 07dbcdb

Please sign in to comment.