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

Let XWIN_CACHE_DIR override cmake cache #73

Closed
shivaraj-bh opened this issue Oct 23, 2023 · 6 comments · Fixed by #74
Closed

Let XWIN_CACHE_DIR override cmake cache #73

shivaraj-bh opened this issue Oct 23, 2023 · 6 comments · Fixed by #74

Comments

@shivaraj-bh
Copy link
Contributor

The <cache-path> in <cache-path>/cargo-xwin/xwin is overridden by XWIN_CACHE_DIR env as defined here:

cargo-xwin/src/common.rs

Lines 78 to 85 in 28cd46e

let xwin_cache_dir = self.xwin_cache_dir.clone().unwrap_or_else(|| {
dirs::cache_dir()
// If the really is no cache dir, cwd will also do
.unwrap_or_else(|| env::current_dir().expect("Failed to get current dir"))
.join(env!("CARGO_PKG_NAME"))
.join("xwin")
});
fs::create_dir_all(&xwin_cache_dir)?;

But the cmake cache (cache-path>/cargo-xwin/cmake) always sets its <cache-path> to the default given by dirs crate, as can be seen here:

cargo-xwin/src/common.rs

Lines 398 to 403 in 28cd46e

fn setup_cmake_toolchain(&self, target: &str, xwin_cache_dir: &Path) -> Result<PathBuf> {
let cache_dir = dirs::cache_dir()
.unwrap_or_else(|| env::current_dir().expect("Failed to get current dir"))
.join(env!("CARGO_PKG_NAME"));
let cmake = cache_dir.join("cmake");
fs::create_dir_all(&cmake)?;

Will a PR here to let XWIN_CACHE_DIR override the <cache-path> for cmake be appreciated?

Context:

I was trying to use cargo-xwin here, and nix doesn't allow the builder write to paths outside build directory -- that's when I observed this bug.

@messense
Copy link
Member

Feel free to send a PR.

@shivaraj-bh shivaraj-bh changed the title Use XWIN_CACHE_DIR for cmake cache Add CMAKE_CACHE_DIR env Oct 26, 2023
@shivaraj-bh
Copy link
Contributor Author

Will a PR here to let XWIN_CACHE_DIR override the for cmake be appreciated?

Edit: CMAKE_CACHE_DIR env should override the <cache-path>

@shivaraj-bh
Copy link
Contributor Author

I am not quite certain about the choice of the name, CMAKE_CACHE_DIR, should it be with XWIN_CMAKE_CACHE_DIR instead?

@messense
Copy link
Member

Can't we just make it consistent with

cargo-xwin/src/common.rs

Lines 78 to 85 in 28cd46e

let xwin_cache_dir = self.xwin_cache_dir.clone().unwrap_or_else(|| {
dirs::cache_dir()
// If the really is no cache dir, cwd will also do
.unwrap_or_else(|| env::current_dir().expect("Failed to get current dir"))
.join(env!("CARGO_PKG_NAME"))
.join("xwin")
});
fs::create_dir_all(&xwin_cache_dir)?;

so that you can override it via XWIN_CACHE_DIR?

@shivaraj-bh
Copy link
Contributor Author

That was my initial thought as well.

If I have to keep it consistent, how does this look?

For xwin's cache:

        let xwin_cache_dir = self.xwin_cache_dir.clone().unwrap_or_else(|| {
            dirs::cache_dir()
                // If the really is no cache dir, cwd will also do
                .unwrap_or_else(|| env::current_dir().expect("Failed to get current dir"))
                .join(env!("CARGO_PKG_NAME"))
        }).join("xwin");
        fs::create_dir_all(&xwin_cache_dir)?;

For cmake's cache:

        let cmake_cache_dir = self.xwin_cache_dir.clone().unwrap_or_else(|| {
            dirs::cache_dir()
                // If the really is no cache dir, cwd will also do
                .unwrap_or_else(|| env::current_dir().expect("Failed to get current dir"))
                .join(env!("CARGO_PKG_NAME"))
        }).join("cmake");
        fs::create_dir_all(&cmake_cache_dir)?;

@messense
Copy link
Member

LGTM.

@shivaraj-bh shivaraj-bh changed the title Add CMAKE_CACHE_DIR env Let XWIN_CACHE_DIR override cmake cache Oct 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants