Skip to content
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.

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

Call out in docs that you can operate on things besides the built-in temp support #48

Open
epage opened this issue Mar 25, 2019 · 3 comments
Labels
enhancement Improve the expected

Comments

@epage
Copy link
Contributor

epage commented Mar 25, 2019

In case someone wants custom temp dir functionality or some other unknown situation.

From @volks73 at #46

I would use assert_fs more if I could upgrade from v0.9. The assert_fs crate wraps the tempfile crate, but does not expose all of the functionality. For example, I need to be able to define a prefix for a temporary directory. The default .tmp prefix is an invalid name for a Cargo package. Windows is also not too keen on file and folder names with leading periods. This means I need to use the tempfile::Builder type to add a custom prefix. There is no Fromtempfile::TempDir trait implementation for assert_fs::TempDir, so I cannot use the tempfile::Builder type to customize a temporary directory and then "cast" it to the assert_fs::TempDir type to gain the assertion-ness and conveniences from the assert_fs crate, nor is there a similar Builder interface available. Thus, I am stuck using v0.9 of assert_fs, which does not wrap tempfile and instead just extends it.

This can be worked around today with ChildPath but the docs never talk about it!

@epage epage added the enhancement Improve the expected label Mar 25, 2019
@volks73
Copy link

volks73 commented Mar 26, 2019

So, the ChildPath workaround would be something like this:

#[test]
fn default_works() {
    let temp_package = common::create_test_package();
    let package = ChildPath::new(temp_package.path());
    // ...
}

which would require updating/modifying every test?

I understand this is a workaround, but it would be nicer if I could just change the create_test_package implementation:

#[allow(dead_code)]
pub fn create_test_package() -> assert_fs::TempDir {
    let temp_dir = tempfile::Builder::new().prefix("cargo_wix_test_").tempdir().unwrap();
    let cargo_init_status = Command::new("cargo")
        .arg("init")
        .arg("--bin")
        .arg("--quiet")
        .arg("--vcs")
        .arg("none")
        .arg(temp_dir.path())
        .status()
        .expect("Creation of test Cargo package");
    assert!(cargo_init_status.success());
    temp_dir.into()
    //
    // or
    // assert_fs::TempDir::from(temp_dir)
    //
    // or do the "upcast" earlier with:
    //
    // let temp_dir = assert_fs::TempDir::from(tempfile::Builder::new().prefix("cargo_wix_test_"));
}

Justification for needing the prefix and "special" temporary directory name is given in #46.

@volks73
Copy link

volks73 commented Sep 3, 2019

I know it has been awhile, but a brief update on this since it is still open.

I have upgraded to v0.11 and changed the create_test_package function to use the --name <NAME> option for the cargo init command to get around the limitations of naming cargo projects/packages with periods as discussion in #46 . The following is the current implementation of the create_test_package function:

#[allow(dead_code)]
pub fn create_test_package() -> TempDir {
    let temp_dir = TempDir::new().unwrap();
    let cargo_init_status = Command::new("cargo")
        .arg("init")
        .arg("--bin")
        .arg("--quiet")
        .arg("--vcs")
        .arg("none")
        .arg("--name")
        .arg(PACKAGE_NAME)
        .arg(temp_dir.path())
        .status()
        .expect("Creation of test Cargo package");
    assert!(cargo_init_status.success());
    temp_dir
}

The PACKAGE_NAME is a constant that can only contain letters or numbers because of naming limitations in the WiX Toolset compiler and linker (candle and light). This avoided having to change all of the tests with the above mentioned workaround using the ChildPath type. Although, I did initially try this and it worked.

Side note, on Windows 10, the temporary directory does have a leading period, but this no longer appears to be a problem. I think there were some recent updates to Windows Explorer that better handle leading periods in file and folder names and any problems I was experiencing with this were specific to an older version of Windows I was runing.

I still think it would be nice to have from conversion functionality from the tempfile::Builder so that the options available within the Builder are available to the assert_fs::TempDir without having to re-implement the Builder functionality. Should I create a separate issue to add this functionality? Would you like/accept a PR with this feature added? I might have some time to take a look into implementing.

@epage
Copy link
Contributor Author

epage commented Sep 3, 2019

I still think it would be nice to have from conversion functionality from the tempfile::Builder so that the options available within the Builder are available to the assert_fs::TempDir without having to re-implement the Builder functionality. Should I create a separate issue to add this functionality? Would you like/accept a PR with this feature added? I might have some time to take a look into implementing.

Could you create a separate issue for that? I'll then copy over relevant parts from #46

I'm still hesitant about exposing tempfile in assert_fss public API.

volks73 added a commit to volks73/cargo-wix that referenced this issue Sep 3, 2019
This required changing the implementation of creating test projects. The `.tmp`
prefix can be used in Windows but the cargo project name cannot. Thus, the
`--name <NAME>` option needed to be used for creating a test package because the
`TempDir` type from the assert_fs package does not support the tempfile TempDir
builder or changing the prefix in newer versions. See Issues [#46] and [#48]
from the `assert_fs` project for more information.

[#46]: assert-rs/assert_fs#46
[#48]: assert-rs/assert_fs#48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve the expected
Projects
None yet
Development

No branches or pull requests

2 participants