Merge #16

16: Add mktemp_d r=azdavis a=azdavis

- Use POSIX date invocations, don't use `--iso` option
  - This might have been part of what was making tests fail on macOS
  - I ran `cargo t` on my macOS machine and tests failed because macOS's default `date` doesn't have the `--iso` option
- Add changelog entry for 0.1.7, 0.1.8 retroactively
- Fix cp doc
  - It acts like shell's `cp` when `dst` is a directory
- Allow acquiring many read locks on the same thread
  - Update the thread-local cache to track readers as well as the writer
- Doc and test internal `gsl` more, note some caveats
- Add `mktemp_d`, doc it
- Update version and release notes
- Ignore `target` files in vscode

Closes #12 , can also probably close #7 after this.

Co-authored-by: Ariel Davis <[email protected]>
Co-authored-by: Aleksey Kladov <[email protected]>

Author: 3 people

.vscode/settings.json

@@ -0,0 +1,5 @@
+{
+  "files.exclude": {
+    "**/target": true
+  }
+}

CHANGELOG.md

@@ -1,5 +1,21 @@
 # Changelog
 
+## 0.1.9
+
+- `mktemp_d` creates an (insecure, world readable) temporary directory.
+- Fix cp docs
+
+## 0.1.8
+
+- Add option to not echo command at all
+- Add option to censor command contents when echoing
+- Add docs
+
+## 0.1.7
+
+- `cp(foo, bar)` copies `foo` _into_ `bar`, if `bar` is an existing directory.
+- Tweak reading API
+
 ## 0.1.6
 
 - `.read()` chomps `\r\n` on Windows.

Cargo.toml

@@ -2,7 +2,7 @@
 name = "xshell"
 description = "Utilities for quick shell scripting in Rust"
 categories = ["development-tools::build-utils", "filesystem"]
-version = "0.1.8"
+version = "0.1.9"
 license = "MIT OR Apache-2.0"
 repository = "https://github.com/matklad/xshell"
 authors = ["Aleksey Kladov <[email protected]>"]
@@ -13,4 +13,4 @@ exclude = [".github/", "bors.toml", "rustfmt.toml", "cbench", "mock_bin/"]
 [workspace]
 
 [dependencies]
-xshell-macros = { version = "=0.1.8", path = "./xshell-macros"}
+xshell-macros = { version = "=0.1.9", path = "./xshell-macros" }

cbench/baseline/src/main.rs

@@ -2,7 +2,7 @@ use std::process::Command;
 
 fn main() {
     let mut cmd = Command::new("date");
-    cmd.arg("--iso");
+    cmd.arg("+%Y-%m-%d");
     let output = cmd.output().unwrap();
     if !output.status.success() {
         panic!("command `{:?}` failed: {}", cmd, output.status);

cbench/ducted/src/main.rs

@@ -1,6 +1,6 @@
 use duct::cmd;
 
 fn main() {
-    let date = cmd!("date", "--iso").read().unwrap();
+    let date = cmd!("date", "+%Y-%m-%d").read().unwrap();
     print!("today is: {}\n", date)
 }

cbench/xshelled/src/main.rs

@@ -1,6 +1,6 @@
 use xshell::cmd;
 
 fn main() {
-    let date = cmd!("date --iso").read().unwrap();
+    let date = cmd!("date +%Y-%m-%d").read().unwrap();
     print!("today is: {}\n", date)
 }

mock_bin/date.rs

@@ -9,7 +9,7 @@ fn main() {
 
 fn try_main() -> io::Result<()> {
     let args = std::env::args().collect::<Vec<_>>();
-    if args != ["date", "--iso"] {
+    if args != ["date", "+%Y-%m-%d"] {
         return Err(io::Error::new(io::ErrorKind::Other, "invalid args"));
     }
     println!("1982-06-25");

src/fs.rs

@@ -1,4 +1,7 @@
-use std::path::{Path, PathBuf};
+use std::{
+    path::{Path, PathBuf},
+    sync::atomic::{AtomicUsize, Ordering},
+};
 
 use crate::{error::fs_err, gsl, Result};
 
@@ -46,9 +49,14 @@ fn _mkdir_p(path: &Path) -> Result<()> {
     with_path(path, std::fs::create_dir_all(path))
 }
 
-/// Copies the file at `src` into the file at `dst`.
+/// Copies `src` into `dst`.
 ///
-/// `dst` must be the path to a file. It will be created if it does not exist.
+/// `src` must be a file, but `dst` need not be. If `dst` is an existing
+/// directory, `src` will be copied into a file in the `dst` directory whose
+/// name is same as that of `src`.
+///
+/// Otherwise, `dst` is a file or does not exist, and `src` will be copied into
+/// it.
 pub fn cp(src: impl AsRef<Path>, dst: impl AsRef<Path>) -> Result<()> {
     _cp(src.as_ref(), dst.as_ref())
 }
@@ -81,6 +89,30 @@ pub fn cwd() -> Result<PathBuf> {
     with_path(&Path::new("."), std::env::current_dir())
 }
 
+/// Creates an empty, world-readable, temporary directory.
+///
+/// Returns a [`TempDir`] value that provides the path of this temporary
+/// directory. When dropped, the temporary directory and all of its contents
+/// will be removed.
+pub fn mktemp_d() -> Result<TempDir> {
+    let _guard = gsl::read();
+    let base = std::env::temp_dir();
+    mkdir_p(&base)?;
+
+    static CNT: AtomicUsize = AtomicUsize::new(0);
+
+    let mut n_try = 0u32;
+    loop {
+        let cnt = CNT.fetch_add(1, Ordering::Relaxed);
+        let path = base.join(format!("xshell-tmp-dir-{}", cnt));
+        match std::fs::create_dir(&path) {
+            Ok(()) => return Ok(TempDir { path }),
+            Err(io_err) if n_try == 1024 => return Err(fs_err(path, io_err)),
+            Err(_) => n_try += 1,
+        }
+    }
+}
+
 fn with_path<T>(path: &Path, res: Result<T, std::io::Error>) -> Result<T> {
     res.map_err(|io_err| fs_err(path.to_path_buf(), io_err))
 }
@@ -110,3 +142,22 @@ fn read_dir_aux(path: &Path) -> std::io::Result<Vec<PathBuf>> {
     res.sort();
     Ok(res)
 }
+
+/// A temporary directory.
+#[derive(Debug)]
+pub struct TempDir {
+    path: PathBuf,
+}
+
+impl TempDir {
+    /// Returns the path of this temporary directory.
+    pub fn path(&self) -> &Path {
+        &self.path
+    }
+}
+
+impl Drop for TempDir {
+    fn drop(&mut self) {
+        let _ = rm_rf(&self.path);
+    }
+}

src/gsl.rs

@@ -1,4 +1,5 @@
-//! Global shell lock
+//! Global shell lock.
+
 use std::{
     cell::Cell,
     mem::MaybeUninit,
@@ -7,29 +8,77 @@ use std::{
     sync::{RwLock, RwLockReadGuard, RwLockWriteGuard},
 };
 
+/// If, on the same thread, there are multiple calls to [`read`] or [`write`],
+/// then the `Guard`s returned should be dropped in the reverse order that they
+/// were acquired.
+///
+/// If this is violated, e.g. in
+///
+/// ```ignore
+/// let w1 = write();
+/// let _w2 = write();
+/// drop(w1);
+/// // try to use a global resource with write privileges
+/// ```
+///
+/// then things won't turn out well, because `_w2` doesn't actually contain a
+/// lock guard.
+#[derive(Debug)]
+pub(crate) struct Guard(Option<Repr>);
+
 #[derive(Debug)]
-pub(crate) struct Guard {
-    r_guard: Option<RwLockReadGuard<'static, ()>>,
-    w_guard: Option<RwLockWriteGuard<'static, ()>>,
+enum Repr {
+    Read(RwLockReadGuard<'static, ()>),
+    Write(RwLockWriteGuard<'static, ()>),
 }
 
+/// Returns a [`Guard`] for write access to global resources.
 pub(crate) fn write() -> Guard {
-    if LOCKED.with(|it| it.get()) {
-        return Guard { r_guard: None, w_guard: None };
+    match CACHE.with(Cell::get) {
+        Cache::Write => {
+            // this thread (and only this thread) can already write. don't try to
+            // acquire another write guard.
+            Guard(None)
+        }
+        Cache::Read(readers) => {
+            assert_eq!(
+                readers, 0,
+                "calling write() with an active read guard on the same thread would deadlock"
+            );
+            let w_guard = static_rw_lock().write().unwrap_or_else(|err| err.into_inner());
+            // note that we have a writer.
+            CACHE.with(|it| it.set(Cache::Write));
+            Guard(Some(Repr::Write(w_guard)))
+        }
     }
-
-    let w_guard = static_rw_lock().write().unwrap_or_else(|err| err.into_inner());
-    LOCKED.with(|it| it.set(true));
-    Guard { w_guard: Some(w_guard), r_guard: None }
 }
 
+/// Returns a [`Guard`] for read access to global resources.
 pub(crate) fn read() -> Guard {
-    if LOCKED.with(|it| it.get()) {
-        return Guard { r_guard: None, w_guard: None };
+    match CACHE.with(Cell::get) {
+        Cache::Write => {
+            // this thread (and only this thread) can already write. it's safe
+            // to allow this thread to read as well, because we won't have
+            // concurrent reads and writes, because we're only working on this
+            // thread.
+            Guard(None)
+        }
+        Cache::Read(readers) => {
+            if readers == 0 {
+                // this thread has no readers or writers. try to acquire the
+                // lock for reading.
+                let r_guard = static_rw_lock().read().unwrap_or_else(|err| err.into_inner());
+                // note that we now have 1 reader.
+                CACHE.with(|it| it.set(Cache::Read(1)));
+                Guard(Some(Repr::Read(r_guard)))
+            } else {
+                // this thread can already read. don't try to acquire another
+                // read guard. also, note that we have another reader.
+                CACHE.with(|it| it.set(Cache::Read(readers + 1)));
+                Guard(None)
+            }
+        }
     }
-
-    let r_guard = static_rw_lock().read().unwrap_or_else(|err| err.into_inner());
-    Guard { w_guard: None, r_guard: Some(r_guard) }
 }
 
 fn static_rw_lock() -> &'static RwLock<()> {
@@ -41,15 +90,69 @@ fn static_rw_lock() -> &'static RwLock<()> {
     }
 }
 
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+enum Cache {
+    Read(usize),
+    Write,
+}
+
 thread_local! {
-    static LOCKED: Cell<bool> = Cell::new(false);
+    static CACHE: Cell<Cache> = Cell::new(Cache::Read(0));
 }
 
 impl Drop for Guard {
     fn drop(&mut self) {
-        if self.w_guard.is_some() {
-            LOCKED.with(|it| it.set(false))
+        match self.0 {
+            Some(Repr::Read(_)) => CACHE.with(|it| {
+                let n = match it.get() {
+                    Cache::Read(n) => n,
+                    Cache::Write => unreachable!("had both a reader and a writer"),
+                };
+                it.set(Cache::Read(n - 1));
+            }),
+            Some(Repr::Write(_)) => CACHE.with(|it| {
+                assert_eq!(it.get(), Cache::Write);
+                it.set(Cache::Read(0));
+            }),
+            None => {}
         }
-        let _ = self.r_guard;
     }
 }
+
+#[test]
+fn read_write_read() {
+    eprintln!("get r1");
+    let r1 = read();
+    eprintln!("got r1");
+    let h = std::thread::spawn(|| {
+        eprintln!("get w1");
+        let w1 = write();
+        eprintln!("got w1");
+        drop(w1);
+        eprintln!("gave w1");
+    });
+    std::thread::sleep(std::time::Duration::from_millis(300));
+    eprintln!("get r2");
+    let r2 = read();
+    eprintln!("got r2");
+    drop(r1);
+    eprintln!("gave r1");
+    drop(r2);
+    eprintln!("gave r2");
+    h.join().unwrap();
+}
+
+#[test]
+fn write_read() {
+    let _w = write();
+    let _r = read();
+}
+
+#[test]
+#[should_panic(
+    expected = "calling write() with an active read guard on the same thread would deadlock"
+)]
+fn read_write() {
+    let _r = read();
+    let _w = write();
+}

src/lib.rs

@@ -42,7 +42,7 @@
 //!
 //! ```
 //! # use xshell::cmd;
-//! let output = cmd!("date --iso").read()?;
+//! let output = cmd!("date +%Y-%m-%d").read()?;
 //! assert!(output.chars().all(|c| "01234567890-".contains(c)));
 //! # Ok::<(), xshell::Error>(())
 //! ```
@@ -177,7 +177,7 @@
 //!
 //! # Implementation details
 //!
-//! The design is heavily inspired by the Juila language:
+//! The design is heavily inspired by the Julia language:
 //!
 //! * [Shelling Out Sucks](https://julialang.org/blog/2012/03/shelling-out-sucks/)
 //! * [Put This In Your Pipe](https://julialang.org/blog/2013/04/put-this-in-your-pipe/)
@@ -190,7 +190,7 @@
 //!
 //! The `cmd!` macro uses a simple proc-macro internally. It doesn't depend on
 //! helper libraries, so the fixed-cost impact on compile times is moderate.
-//! Compiling a trivial program with `cmd!("date --iso")` takes one second.
+//! Compiling a trivial program with `cmd!("date +%Y-%m-%d")` takes one second.
 //! Equivalent program using only `std::process::Command` compiles in 0.25
 //! seconds.
 //!
@@ -261,7 +261,7 @@ pub use xshell_macros::__cmd;
 pub use crate::{
     env::{pushd, pushenv, Pushd, Pushenv},
     error::{Error, Result},
-    fs::{cp, cwd, mkdir_p, read_dir, read_file, rm_rf, write_file},
+    fs::{cp, cwd, mkdir_p, mktemp_d, read_dir, read_file, rm_rf, write_file, TempDir},
 };
 
 /// Constructs a [`Cmd`] from the given string.