Skip to content

Less copying during dist installation #1744

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Apr 14, 2019
Merged

Less copying during dist installation #1744

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 10 additions & 1 deletion src/dist/component/components.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,16 @@ impl<'a> ComponentBuilder<'a> {
.push(ComponentPart("dir".to_owned(), path.clone()));
self.tx.copy_dir(&self.name, path, src)
}

pub fn move_file(&mut self, path: PathBuf, src: &Path) -> Result<()> {
self.parts
.push(ComponentPart("file".to_owned(), path.clone()));
self.tx.move_file(&self.name, path, src)
}
pub fn move_dir(&mut self, path: PathBuf, src: &Path) -> Result<()> {
self.parts
.push(ComponentPart("dir".to_owned(), path.clone()));
self.tx.move_dir(&self.name, path, src)
}
pub fn finish(mut self) -> Result<Transaction<'a>> {
// Write component manifest
let path = self.components.rel_component_manifest(&self.name);
Expand Down
22 changes: 18 additions & 4 deletions src/dist/component/package.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,17 +35,19 @@ pub trait Package: fmt::Debug {
pub struct DirectoryPackage {
path: PathBuf,
components: HashSet<String>,
copy: bool,
}

impl DirectoryPackage {
pub fn new(path: PathBuf) -> Result<Self> {
pub fn new(path: PathBuf, copy: bool) -> Result<Self> {
validate_installer_version(&path)?;

let content = utils::read_file("package components", &path.join("components"))?;
let components = content.lines().map(|l| l.to_owned()).collect();
Ok(DirectoryPackage {
path: path,
components: components,
copy: copy,
})
}
}
Expand Down Expand Up @@ -97,8 +99,20 @@ impl Package for DirectoryPackage {
let src_path = root.join(&path);

match &*part.0 {
"file" => builder.copy_file(path.clone(), &src_path)?,
"dir" => builder.copy_dir(path.clone(), &src_path)?,
"file" => {
if self.copy {
builder.copy_file(path.clone(), &src_path)?
} else {
builder.move_file(path.clone(), &src_path)?
}
}
"dir" => {
if self.copy {
builder.copy_dir(path.clone(), &src_path)?
} else {
builder.move_dir(path.clone(), &src_path)?
}
}
_ => return Err(ErrorKind::CorruptComponent(name.to_owned()).into()),
}

Expand Down Expand Up @@ -187,7 +201,7 @@ impl<'a> TarPackage<'a> {
unpack_without_first_dir(&mut archive, &*temp_dir)?;

Ok(TarPackage(
DirectoryPackage::new(temp_dir.to_owned())?,
DirectoryPackage::new(temp_dir.to_owned(), false)?,
temp_dir,
))
}
Expand Down
89 changes: 55 additions & 34 deletions src/dist/component/transaction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,22 @@ impl<'a> Transaction<'a> {
Ok(())
}

/// Move a file to a relative path of the install prefix.
pub fn move_file(&mut self, component: &str, relpath: PathBuf, src: &Path) -> Result<()> {
assert!(relpath.is_relative());
let item = ChangedItem::move_file(&self.prefix, component, relpath, src)?;
self.change(item);
Ok(())
}

/// Recursively move a directory to a relative path of the install prefix.
pub fn move_dir(&mut self, component: &str, relpath: PathBuf, src: &Path) -> Result<()> {
assert!(relpath.is_relative());
let item = ChangedItem::move_dir(&self.prefix, component, relpath, src)?;
self.change(item);
Ok(())
}

pub fn temp(&self) -> &'a temp::Cfg {
self.temp_cfg
}
Expand Down Expand Up @@ -195,8 +211,12 @@ impl<'a> ChangedItem<'a> {
}
Ok(())
}
fn add_file(prefix: &InstallPrefix, component: &str, relpath: PathBuf) -> Result<(Self, File)> {
let abs_path = prefix.abs_path(&relpath);
fn dest_abs_path(
prefix: &InstallPrefix,
component: &str,
relpath: &PathBuf,
) -> Result<PathBuf> {
let abs_path = prefix.abs_path(relpath);
if utils::path_exists(&abs_path) {
Err(ErrorKind::ComponentConflict {
name: component.to_owned(),
Expand All @@ -207,53 +227,34 @@ impl<'a> ChangedItem<'a> {
if let Some(p) = abs_path.parent() {
utils::ensure_dir_exists("component", p, &|_| ())?;
}
let file = File::create(&abs_path)
.chain_err(|| format!("error creating file '{}'", abs_path.display()))?;

Ok((ChangedItem::AddedFile(relpath), file))
Ok(abs_path)
}
}
fn add_file(prefix: &InstallPrefix, component: &str, relpath: PathBuf) -> Result<(Self, File)> {
let abs_path = ChangedItem::dest_abs_path(prefix, component, &relpath)?;
let file = File::create(&abs_path)
.chain_err(|| format!("error creating file '{}'", abs_path.display()))?;
Ok((ChangedItem::AddedFile(relpath), file))
}
fn copy_file(
prefix: &InstallPrefix,
component: &str,
relpath: PathBuf,
src: &Path,
) -> Result<Self> {
let abs_path = prefix.abs_path(&relpath);
if utils::path_exists(&abs_path) {
Err(ErrorKind::ComponentConflict {
name: component.to_owned(),
path: relpath.clone(),
}
.into())
} else {
if let Some(p) = abs_path.parent() {
utils::ensure_dir_exists("component", p, &|_| ())?;
}
utils::copy_file(src, &abs_path)?;
Ok(ChangedItem::AddedFile(relpath))
}
let abs_path = ChangedItem::dest_abs_path(prefix, component, &relpath)?;
utils::copy_file(src, &abs_path)?;
Ok(ChangedItem::AddedFile(relpath))
}
fn copy_dir(
prefix: &InstallPrefix,
component: &str,
relpath: PathBuf,
src: &Path,
) -> Result<Self> {
let abs_path = prefix.abs_path(&relpath);
if utils::path_exists(&abs_path) {
Err(ErrorKind::ComponentConflict {
name: component.to_owned(),
path: relpath.clone(),
}
.into())
} else {
if let Some(p) = abs_path.parent() {
utils::ensure_dir_exists("component", p, &|_| ())?;
}
utils::copy_dir(src, &abs_path, &|_| ())?;
Ok(ChangedItem::AddedDir(relpath))
}
let abs_path = ChangedItem::dest_abs_path(prefix, component, &relpath)?;
utils::copy_dir(src, &abs_path, &|_| ())?;
Ok(ChangedItem::AddedDir(relpath))
}
fn remove_file(
prefix: &InstallPrefix,
Expand Down Expand Up @@ -311,4 +312,24 @@ impl<'a> ChangedItem<'a> {
Ok(ChangedItem::ModifiedFile(relpath, None))
}
}
fn move_file(
prefix: &InstallPrefix,
component: &str,
relpath: PathBuf,
src: &Path,
) -> Result<Self> {
let abs_path = ChangedItem::dest_abs_path(prefix, component, &relpath)?;
utils::rename_file("component", src, &abs_path)?;
Ok(ChangedItem::AddedFile(relpath))
}
fn move_dir(
prefix: &InstallPrefix,
component: &str,
relpath: PathBuf,
src: &Path,
) -> Result<Self> {
let abs_path = ChangedItem::dest_abs_path(prefix, component, &relpath)?;
utils::rename_dir("component", src, &abs_path)?;
Ok(ChangedItem::AddedDir(relpath))
}
}
16 changes: 8 additions & 8 deletions tests/dist_install.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ fn package_contains() {

mock.build(tempdir.path());

let package = DirectoryPackage::new(tempdir.path().to_owned()).unwrap();
let package = DirectoryPackage::new(tempdir.path().to_owned(), true).unwrap();
assert!(package.contains("mycomponent", None));
assert!(package.contains("mycomponent2", None));
}
Expand All @@ -88,7 +88,7 @@ fn package_bad_version() {
let mut ver = File::create(tempdir.path().join("rust-installer-version")).unwrap();
writeln!(ver, "100").unwrap();

assert!(DirectoryPackage::new(tempdir.path().to_owned()).is_err());
assert!(DirectoryPackage::new(tempdir.path().to_owned(), true).is_err());
}

#[test]
Expand Down Expand Up @@ -122,7 +122,7 @@ fn basic_install() {

let components = Components::open(prefix.clone()).unwrap();

let pkg = DirectoryPackage::new(pkgdir.path().to_owned()).unwrap();
let pkg = DirectoryPackage::new(pkgdir.path().to_owned(), true).unwrap();

let tx = pkg.install(&components, "mycomponent", None, tx).unwrap();
tx.commit();
Expand Down Expand Up @@ -168,7 +168,7 @@ fn multiple_component_install() {

let components = Components::open(prefix.clone()).unwrap();

let pkg = DirectoryPackage::new(pkgdir.path().to_owned()).unwrap();
let pkg = DirectoryPackage::new(pkgdir.path().to_owned(), true).unwrap();

let tx = pkg.install(&components, "mycomponent", None, tx).unwrap();
let tx = pkg.install(&components, "mycomponent2", None, tx).unwrap();
Expand Down Expand Up @@ -218,7 +218,7 @@ fn uninstall() {

let components = Components::open(prefix.clone()).unwrap();

let pkg = DirectoryPackage::new(pkgdir.path().to_owned()).unwrap();
let pkg = DirectoryPackage::new(pkgdir.path().to_owned(), true).unwrap();

let tx = pkg.install(&components, "mycomponent", None, tx).unwrap();
let tx = pkg.install(&components, "mycomponent2", None, tx).unwrap();
Expand Down Expand Up @@ -275,7 +275,7 @@ fn component_bad_version() {

let components = Components::open(prefix.clone()).unwrap();

let pkg = DirectoryPackage::new(pkgdir.path().to_owned()).unwrap();
let pkg = DirectoryPackage::new(pkgdir.path().to_owned(), true).unwrap();

let tx = pkg.install(&components, "mycomponent", None, tx).unwrap();
tx.commit();
Expand Down Expand Up @@ -336,7 +336,7 @@ fn unix_permissions() {

let components = Components::open(prefix.clone()).unwrap();

let pkg = DirectoryPackage::new(pkgdir.path().to_owned()).unwrap();
let pkg = DirectoryPackage::new(pkgdir.path().to_owned(), true).unwrap();

let tx = pkg.install(&components, "mycomponent", None, tx).unwrap();
tx.commit();
Expand Down Expand Up @@ -421,7 +421,7 @@ fn install_to_prefix_that_does_not_exist() {

let components = Components::open(prefix.clone()).unwrap();

let pkg = DirectoryPackage::new(pkgdir.path().to_owned()).unwrap();
let pkg = DirectoryPackage::new(pkgdir.path().to_owned(), true).unwrap();

let tx = pkg.install(&components, "mycomponent", None, tx).unwrap();
tx.commit();
Expand Down