From 3745bd2d0f8d482e6f65abe085057fc4f191ea86 Mon Sep 17 00:00:00 2001 From: Scott Macfarlane Date: Tue, 6 Aug 2019 14:47:04 -0700 Subject: [PATCH] Revert "redux: cross platform binstubs with package environment" Signed-off-by: Scott Macfarlane --- components/hab/src/command/pkg/binlink.rs | 308 +++++++-------------- components/hab/src/error.rs | 8 +- components/hab/src/lib.rs | 1 - components/hab/static/template_binstub.bat | 4 - components/hab/static/template_binstub.sh | 4 - components/pkg-export-docker/src/build.rs | 24 +- test/shellcheck.sh | 2 +- 7 files changed, 119 insertions(+), 232 deletions(-) delete mode 100644 components/hab/static/template_binstub.bat delete mode 100644 components/hab/static/template_binstub.sh diff --git a/components/hab/src/command/pkg/binlink.rs b/components/hab/src/command/pkg/binlink.rs index d2dee6c08d..b007047d64 100644 --- a/components/hab/src/command/pkg/binlink.rs +++ b/components/hab/src/command/pkg/binlink.rs @@ -1,9 +1,10 @@ -use std::{collections::HashMap, - env, - fs::{self, - File}, - io::{BufRead, - BufReader}, +#[cfg(windows)] +use std::fs::File; +#[cfg(windows)] +use std::io::{BufRead, + BufReader}; +use std::{env, + fs, path::{Path, PathBuf}}; @@ -18,19 +19,7 @@ use crate::error::{Error, Result}; #[cfg(windows)] -const COMMENT_MARKER: &str = "REM"; -#[cfg(unix)] -const COMMENT_MARKER: &str = "#"; -#[cfg(windows)] -const PATH_SEPARATOR: char = ';'; -#[cfg(unix)] -const PATH_SEPARATOR: char = ':'; -#[cfg(windows)] -const SET_OR_EXPORT: &str = "SET"; -#[cfg(unix)] -const SET_OR_EXPORT: &str = "export"; - -const DEFAULT_INTERPRETER: &str = "/bin/sh"; +const BAT_COMMENT_MARKER: &str = "REM"; pub fn start(ui: &mut UI, ident: &PackageIdent, @@ -41,10 +30,10 @@ pub fn start(ui: &mut UI, -> Result<()> { let dst_path = fs_root_path.join(dest_path.strip_prefix("/")?); ui.begin(format!("Binlinking {} from {} into {}", - binary, - ident, + &binary, + &ident, dst_path.display()))?; - let pkg_install = PackageInstall::load(ident, Some(fs_root_path))?; + let pkg_install = PackageInstall::load(&ident, Some(fs_root_path))?; let mut src = match hfs::find_command_in_pkg(binary, &pkg_install, fs_root_path)? { Some(c) => c, None => { @@ -61,28 +50,29 @@ pub fn start(ui: &mut UI, format!("parent directory {}", dst_path.display()))?; fs::create_dir_all(&dst_path)? } - - let binlink = Binlink::new(&src, &dst_path, &PathBuf::from(DEFAULT_INTERPRETER))?; + let binlink = Binlink::new(&src, &dst_path)?; let ui_binlinked = format!("Binlinked {} from {} to {}", - binary, - pkg_install.ident(), - binlink.link.display(),); - match Binlink::from_file(&binlink.link) { + &binary, + &pkg_install.ident(), + &binlink.dest.display(),); + match Binlink::from_file(&binlink.dest) { Ok(link) => { - if force { - fs::remove_file(link.link)?; - binlink.link(pkg_install.environment_for_command()?)?; - ui.end(ui_binlinked)?; - } else { + if force && link.src != src { + fs::remove_file(&link.dest)?; + binlink.link()?; + ui.end(&ui_binlinked)?; + } else if link.src != src { ui.warn(format!("Skipping binlink because {} already exists at {}. Use --force \ to overwrite", - binary, - link.link.display(),))?; + &binary, + &link.dest.display(),))?; + } else { + ui.end(&ui_binlinked)?; } } Err(_) => { - binlink.link(pkg_install.environment_for_command()?)?; - ui.end(ui_binlinked)?; + binlink.link()?; + ui.end(&ui_binlinked)?; } } @@ -101,7 +91,7 @@ pub fn binlink_all_in_pkg(ui: &mut UI, fs_root_path: &Path, force: bool) -> Result<()> { - let pkg_path = PackageInstall::load(pkg_ident, Some(fs_root_path))?; + let pkg_path = PackageInstall::load(&pkg_ident, Some(fs_root_path))?; for bin_path in pkg_path.paths()? { for bin in fs::read_dir(fs_root_path.join(bin_path.strip_prefix("/")?))? { let bin_file = bin?; @@ -139,167 +129,95 @@ pub fn binlink_all_in_pkg(ui: &mut UI, continue; } }; - self::start(ui, pkg_ident, &bin_name, dest_path, fs_root_path, force)?; + self::start(ui, &pkg_ident, &bin_name, dest_path, &fs_root_path, force)?; } } Ok(()) } -fn is_dest_on_path(dest_dir: &Path) -> bool { +fn is_dest_on_path>(dest_dir: T) -> bool { if let Some(val) = env::var_os("PATH") { - env::split_paths(&val).any(|p| p == dest_dir) + env::split_paths(&val).any(|p| p == dest_dir.as_ref()) } else { false } } -#[allow(dead_code)] struct Binlink { - link: PathBuf, - target: PathBuf, - default_interpreter: PathBuf, + dest: PathBuf, + src: PathBuf, } +#[cfg(not(target_os = "windows"))] impl Binlink { - pub fn new(target: &Path, link: &Path, default_interpreter: &Path) -> Result { - Ok(Self { link: Self::binstub_path(&target, link)?, - target: target.to_path_buf(), - default_interpreter: default_interpreter.to_path_buf(), }) - } - - pub fn from_file(path: &Path) -> Result { - // its possible the link could already exist as a symlink either - // because a previous version of habitat did the binlinking or - // the symlink created outside of habitat - fs::read_link(path).map(|target| Self::with_default_interpreter(target, path.into())) - .or_else(|_| Self::try_from_script(path)) - } - - fn try_from_script(path: &Path) -> Result { - let file = File::open(path)?; - for line in BufReader::new(file).lines() { - let ln = line?; - if ln.to_uppercase().starts_with(COMMENT_MARKER) { - let (_, rest) = ln.split_at(COMMENT_MARKER.len()); - if let Some(target) = Self::get_target_from_toml(rest) { - return Ok(Self::with_default_interpreter(target.into(), path.into())); - } - } - } - Err(Error::CannotParseBinlinkTarget(path.to_path_buf())) - } - - fn with_default_interpreter(target: PathBuf, link: PathBuf) -> Self { - Self { link, - target, - default_interpreter: PathBuf::from(DEFAULT_INTERPRETER) } + pub fn new>(src: T, dest_dir: T) -> Result { + let bin_name = match src.as_ref().file_name() { + Some(name) => name, + None => return Err(Error::CannotParseBinlinkSource(src.as_ref().to_path_buf())), + }; + + Ok(Self { dest: dest_dir.as_ref().join(bin_name), + src: src.as_ref().to_path_buf(), }) } - fn get_target_from_toml(toml: &str) -> Option { - toml.parse() - .ok() - .as_ref() - .and_then(toml::value::Value::as_table) - // Prior to 0.84.0, we used 'source' so we fallback - // to 'source' for links created with older versions - .and_then(|toml_table| toml_table.get("target").or_else(|| toml_table.get("source"))) - .and_then(toml::value::Value::as_str) - .map(String::from) + pub fn from_file>(dest: T) -> Result { + Ok(Binlink { dest: dest.as_ref().to_path_buf(), + src: fs::read_link(&dest)?, }) } - pub fn link(&self, env: HashMap) -> Result<()> { - #[cfg(windows)] - { - fs::write(&self.link, self.stub_template(env)?.as_bytes())?; - Ok(()) - } + pub fn link(&self) -> Result<()> { + use crate::hcore::os::filesystem; - #[cfg(unix)] - { - use std::{io::Write, - os::unix::fs::OpenOptionsExt}; - fs::OpenOptions::new().create(true) - .write(true) - .truncate(true) - .mode(0o775) - .open(&self.link)? - .write_all(self.stub_template(env)?.as_bytes())?; - Ok(()) - } + filesystem::symlink(&self.src, &self.dest)?; + Ok(()) } +} - fn binstub_path(target: &Path, link: &Path) -> Result { - #[cfg(windows)] - { - let bin_name = match target.file_stem() { - Some(name) => name, - None => return Err(Error::CannotParseBinlinkTarget(target.to_path_buf())), - }; - let mut path = link.join(bin_name); - path.set_extension("bat"); - Ok(path) - } - - #[cfg(unix)] - match target.file_name() { - Some(name) => Ok(link.join(name)), - None => Err(Error::CannotParseBinlinkTarget(target.to_path_buf())), - } +#[cfg(target_os = "windows")] +impl Binlink { + pub fn new>(src: T, dest_dir: T) -> Result { + let bin_name = match src.as_ref().file_stem() { + Some(name) => name, + None => return Err(Error::CannotParseBinlinkSource(src.as_ref().to_path_buf())), + }; + let mut path = dest_dir.as_ref().join(bin_name); + path.set_extension("bat"); + + Ok(Binlink { dest: path, + src: src.as_ref().to_path_buf(), }) } - fn stub_template(&self, env: HashMap) -> Result { - let mut exports = String::new(); - for (key, mut value) in env.into_iter() { - if key == "PATH" { - value.push(PATH_SEPARATOR); - value.push_str(&Self::interpolated_var("PATH")); - } - exports.push_str(&format!("{} {}={}\n", SET_OR_EXPORT, key, value)); - } - - #[cfg(windows)] - { - Ok(format!(include_str!("../../../static/template_binstub.\ - bat"), - target = self.target.display(), - env = exports)) - } + pub fn from_file>(path: T) -> Result { + use toml::Value::Table; - #[cfg(unix)] - { - // We need to prevent the binstub from using itself as an interpreter. - // On linux we use `/bin/sh` as the binstub interpreter and if this package - // includes a `sh` binary, we risk a circularly linked interpreter. We can - // detect if the link path is the same as the interpreter and if so, use the - // target binary as the interpreter. - let interpreter = { - if self.default_interpreter.canonicalize()? == self.link { - &self.target - } else { - &self.default_interpreter + let file = File::open(&path)?; + for line in BufReader::new(file).lines() { + let ln = line?; + if ln.to_uppercase().starts_with(BAT_COMMENT_MARKER) { + let (_, rest) = ln.split_at(BAT_COMMENT_MARKER.len()); + if let Ok(Table(toml_exp)) = rest.parse() { + if let Some(src) = toml_exp.get("source") { + if let Some(val) = src.as_str() { + return Ok(Binlink { dest: path.as_ref().to_path_buf(), + src: PathBuf::from(val), }); + } + } } - }; - - Ok(format!(include_str!("../../../static/template_binstub.sh"), - target = self.target.display(), - env = exports, - interpreter = interpreter.display())) + } } + Err(Error::CannotParseBinlinkSource(path.as_ref().to_path_buf())) } - fn interpolated_var(name: &str) -> String { - #[cfg(windows)] - { - format!("%{}%", name) - } - #[cfg(unix)] - { - format!(r#""${}""#, name) - } + pub fn link(&self) -> Result<()> { + let template = format!("@echo off\nREM source='{0}'\n\"{0}\" %*", + self.src.display()); + fs::write(&self.dest, template)?; + Ok(()) } } #[cfg(test)] +#[cfg(any(target_os = "linux", target_os = "windows"))] mod test { use std::{collections::HashMap, env, @@ -351,9 +269,6 @@ mod test { let hypnoanalyze_link = "hypnoanalyze.exe"; #[cfg(target_os = "windows")] let hypnoanalyze_link = "hypnoanalyze.bat"; - let curr_path = format!("{}{}", - super::PATH_SEPARATOR, - Binlink::interpolated_var("PATH")); start(&mut ui, &ident, @@ -361,10 +276,9 @@ mod test { &dst_path, rootfs.path(), force).unwrap(); - assert!(fs::read_to_string(rootfs_bin_dir.join(magicate_link)).unwrap().contains(&format!("PATH={}{}", rootfs_src_dir.to_string_lossy(), curr_path))); assert_eq!(rootfs_src_dir.join("magicate.exe"), - Binlink::from_file(&rootfs_bin_dir.join(magicate_link)).unwrap() - .target); + Binlink::from_file(rootfs_bin_dir.join(magicate_link)).unwrap() + .src); start(&mut ui, &ident, @@ -372,29 +286,9 @@ mod test { &dst_path, rootfs.path(), force).unwrap(); - assert!(fs::read_to_string(rootfs_bin_dir.join(hypnoanalyze_link)).unwrap().contains(&format!("PATH={}{}", rootfs_src_dir.to_string_lossy(), curr_path))); assert_eq!(rootfs_src_dir.join("hypnoanalyze.exe"), - Binlink::from_file(&rootfs_bin_dir.join(hypnoanalyze_link)).unwrap() - .target); - } - - #[test] - #[cfg(unix)] - fn link_points_to_interpreter() { - use std::path::PathBuf; - - let rootfs = TempDir::new().unwrap(); - let rootfs_bin_dir = rootfs.path().join("bin"); - let rootfs_bin_sh = rootfs_bin_dir.join("sh"); - fs::create_dir_all(&rootfs_bin_dir).unwrap(); - - let link = Binlink::new(&PathBuf::from("/src/binary/sh"), - &rootfs_bin_dir, - &rootfs_bin_sh).unwrap(); - link.link(HashMap::new()).unwrap(); - - assert!(fs::read_to_string(rootfs_bin_sh).unwrap() - .contains("#!/src/binary/sh")); + Binlink::from_file(rootfs_bin_dir.join(hypnoanalyze_link)).unwrap() + .src); } #[test] @@ -431,14 +325,14 @@ mod test { binlink_all_in_pkg(&mut ui, &ident, &dst_path, rootfs.path(), force).unwrap(); assert_eq!(rootfs_src_dir.join("bin/magicate.exe"), - Binlink::from_file(&rootfs_bin_dir.join(magicate_link)).unwrap() - .target); + Binlink::from_file(rootfs_bin_dir.join(magicate_link)).unwrap() + .src); assert_eq!(rootfs_src_dir.join("bin/hypnoanalyze.exe"), - Binlink::from_file(&rootfs_bin_dir.join(hypnoanalyze_link)).unwrap() - .target); + Binlink::from_file(rootfs_bin_dir.join(hypnoanalyze_link)).unwrap() + .src); assert_eq!(rootfs_src_dir.join("sbin/securitize.exe"), - Binlink::from_file(&rootfs_bin_dir.join(securitize_link)).unwrap() - .target); + Binlink::from_file(rootfs_bin_dir.join(securitize_link)).unwrap() + .src); } #[test] @@ -461,9 +355,9 @@ mod test { binlink_all_in_pkg(&mut ui, &ident, &dst_path, rootfs.path(), force).unwrap(); assert_eq!(rootfs_src_dir.join("bin/magicate.exe"), - Binlink::from_file(&rootfs_bin_dir.join("magicate.bat")).unwrap() - .target); - assert!(Binlink::from_file(&rootfs_bin_dir.join("hypnoanalyze.bat")).is_err()); + Binlink::from_file(rootfs_bin_dir.join("magicate.bat")).unwrap() + .src); + assert!(Binlink::from_file(rootfs_bin_dir.join("hypnoanalyze.bat")).is_err()); } #[test] @@ -503,11 +397,11 @@ mod test { binlink_all_in_pkg(&mut ui, &ident, &dst_path, rootfs.path(), force).unwrap(); assert_eq!(rootfs_src_dir.join("bin/magicate.exe"), - Binlink::from_file(&rootfs_bin_dir.join(magicate_link)).unwrap() - .target); + Binlink::from_file(rootfs_bin_dir.join(magicate_link)).unwrap() + .src); assert_eq!(rootfs_src_dir.join("bin/moar/bonus-round.exe"), - Binlink::from_file(&rootfs_bin_dir.join(bonus_round_link)).unwrap() - .target); + Binlink::from_file(rootfs_bin_dir.join(bonus_round_link)).unwrap() + .src); } fn ui() -> (UI, OutputBuffer, OutputBuffer) { diff --git a/components/hab/src/error.rs b/components/hab/src/error.rs index e2973d5de6..a5324ab0c5 100644 --- a/components/hab/src/error.rs +++ b/components/hab/src/error.rs @@ -26,7 +26,7 @@ pub enum Error { ArgumentError(&'static str), ButterflyError(String), CannotParseBinlinkBinaryName(PathBuf), - CannotParseBinlinkTarget(PathBuf), + CannotParseBinlinkSource(PathBuf), CannotRemoveDockerStudio, CannotRemoveFromChannel((String, String)), CannotRemovePackage(hcore::package::PackageIdent, usize), @@ -77,8 +77,8 @@ impl fmt::Display for Error { Error::CannotParseBinlinkBinaryName(ref p) => { format!("Cannot parse binlink binary name from {}.", p.display()) } - Error::CannotParseBinlinkTarget(ref p) => { - format!("Cannot parse binlink target path from {}.", p.display()) + Error::CannotParseBinlinkSource(ref p) => { + format!("Cannot parse binlink source path from {}.", p.display()) } Error::CannotRemoveDockerStudio => { "Docker Studios are not persistent and cannot be removed".to_string() @@ -186,7 +186,7 @@ impl error::Error for Error { Error::ArgumentError(_) => "There was an error parsing an error or with it's value", Error::ButterflyError(_) => "Butterfly has had an error", Error::CannotParseBinlinkBinaryName(_) => "Cannot parse binlink binary name", - Error::CannotParseBinlinkTarget(_) => "Cannot parse binlink target path", + Error::CannotParseBinlinkSource(_) => "Cannot parse binlink source path", Error::CannotRemoveFromChannel(_) => { "Package cannot be removed from the specified channel" } diff --git a/components/hab/src/lib.rs b/components/hab/src/lib.rs index 0db569d46c..6bebf67474 100644 --- a/components/hab/src/lib.rs +++ b/components/hab/src/lib.rs @@ -16,7 +16,6 @@ extern crate log; extern crate serde_derive; extern crate serde_json; -extern crate toml; #[cfg(windows)] extern crate widestring; diff --git a/components/hab/static/template_binstub.bat b/components/hab/static/template_binstub.bat deleted file mode 100644 index 9c5e3d9209..0000000000 --- a/components/hab/static/template_binstub.bat +++ /dev/null @@ -1,4 +0,0 @@ -@echo off -REM target='{target}' -{env} -"{target}" %* diff --git a/components/hab/static/template_binstub.sh b/components/hab/static/template_binstub.sh deleted file mode 100644 index 69386d64a1..0000000000 --- a/components/hab/static/template_binstub.sh +++ /dev/null @@ -1,4 +0,0 @@ -#!{interpreter} -# target='{target}' -{env} -exec {target} "$@" diff --git a/components/pkg-export-docker/src/build.rs b/components/pkg-export-docker/src/build.rs index ca78ead9bf..02dd923a51 100644 --- a/components/pkg-export-docker/src/build.rs +++ b/components/pkg-export-docker/src/build.rs @@ -888,23 +888,25 @@ mod test { #[cfg(unix)] #[test] fn link_binaries() { - use std::fs; - let rootfs = TempDir::new().unwrap(); let (mut ui, ..) = ui(); let base_pkgs = base_pkgs(rootfs.path()); build_spec().link_binaries(&mut ui, rootfs.path(), &base_pkgs) .unwrap(); - assert!(fs::read_to_string(rootfs.path().join("bin/busybox")).unwrap().contains(hcore::fs::pkg_install_path(base_pkgs.busybox.as_ref().unwrap(), - None::<&Path>).join("bin/busybox").to_str().unwrap()), - "busybox program is binlinked into /bin"); - assert!(fs::read_to_string(rootfs.path().join("bin/sh")).unwrap().contains(hcore::fs::pkg_install_path(&base_pkgs.busybox.unwrap(), - None::<&Path>).join("bin/sh").to_str().unwrap()), - "busybox's sh program is binlinked into /bin"); - assert!(fs::read_to_string(rootfs.path().join("bin/hab")).unwrap().contains(hcore::fs::pkg_install_path(&base_pkgs.hab, - None::<&Path>).join("bin/hab").to_str().unwrap()), - "hab program is binlinked into /bin"); + assert_eq!(hcore::fs::pkg_install_path(base_pkgs.busybox.as_ref().unwrap(), + None::<&Path>).join("bin/busybox"), + rootfs.path().join("bin/busybox").read_link().unwrap(), + "busybox program is symlinked into /bin"); + assert_eq!( + hcore::fs::pkg_install_path(&base_pkgs.busybox.unwrap(), None::<&Path>) + .join("bin/sh"), + rootfs.path().join("bin/sh").read_link().unwrap(), + "busybox's sh program is symlinked into /bin" + ); + assert_eq!(hcore::fs::pkg_install_path(&base_pkgs.hab, None::<&Path>).join("bin/hab"), + rootfs.path().join("bin/hab").read_link().unwrap(), + "hab program is symlinked into /bin"); } #[cfg(unix)] diff --git a/test/shellcheck.sh b/test/shellcheck.sh index 4331e7dc43..fb25d65184 100755 --- a/test/shellcheck.sh +++ b/test/shellcheck.sh @@ -27,7 +27,7 @@ find . -type f \ -or -exec sh -c 'file -b "$1" | grep -q "shell script"' -- {} \; \) \ -and \! -path "*.sample" \ -and \! -path "*.ps1" \ - -and \! -path "./components/hab/static/*" \ + -and \! -path "./components/hab/static/template_plan.sh" \ -and \! -path "./target/*" \ -and \! -path "./test/integration/helpers.bash" \ -and \! -path "./test/integration/test_helper/bats-assert/*" \