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

Solve hardlink on different disk issue and add support for incremental compiles on windows #101

Merged
merged 5 commits into from
May 12, 2023

Conversation

ScSofts
Copy link
Contributor

@ScSofts ScSofts commented May 11, 2023

I've fixed the hardlink issue and added support for incremental build on windows.

@TomieAi
Copy link

TomieAi commented May 11, 2023

nice

@bbqsrc
Copy link
Owner

bbqsrc commented May 11, 2023

I purposely used hard links to stop the disk piling up with space-taking copies of the binary, and adding anything to the target directory to remove the risk of future breakages.

The way you've implemented it will check if something exists, so if an older version of cargo-ndk was copied there and someone didn't do a clean, it'll use that and potentially introduce other bugs.

So here's a compromise I'd propose:

  • Use target/.cargo-ndk and create it with std::fs::create_dir_all(...), and panic if it fails as it should not be possible for that to fail.
  • Attempt to hard link, and if there's an error, attempt to copy instead. Fail on copying error.
  • At the end of the build process, delete the target/.cargo-ndk directory and ignore any errors.

Does this sound sensible?

@ScSofts
Copy link
Contributor Author

ScSofts commented May 11, 2023

I admit I was a bit thoughtless. I will make modifications and then reissue the pull request.

@ScSofts ScSofts closed this May 11, 2023
@bbqsrc
Copy link
Owner

bbqsrc commented May 11, 2023

Not thoughtless, a PR is a good way to make a suggestion. Thank you for being receptive to feedback. 😄

@ScSofts
Copy link
Contributor Author

ScSofts commented May 11, 2023

Since I want incremental build support, I insist on retaining target/.cargo-ndk directory. But I took another approach to avoid the version problem:

Version Checking

// main.rs
// for version check.
    if let Some(_) = args().find(|x| x == "--cargo-ndk-version"){
        print!("{}", env!("CARGO_PKG_VERSION"));
        exit(0);
    }
if executable.exists(){
                // check for executable version.
                if let Ok(Output{ stdout, .. }) = Command::new(&executable)
                    .arg("--cargo-ndk-version")
                    .output(){
                    let ver_tools = Version::parse(String::from_utf8(stdout).unwrap().as_str());
                    let current = Version::parse(env!("CARGO_PKG_VERSION")).unwrap();
                    if let Ok(ver_tools) = ver_tools{
                        if ver_tools == current {
                            debug!("Ignoring {:?} ", &executable);
                            continue;
                        }else{
                            debug!("Removing {:?} ", &executable);
                            std::fs::remove_file(&executable)
                                .expect(format!("Failed to remove {:?}", &executable).as_str());
                        }
                    }
                }

Do you think this modification is acceptable? ❓

@ScSofts ScSofts reopened this May 11, 2023
@ScSofts
Copy link
Contributor Author

ScSofts commented May 11, 2023

I've made several tests localy, it works as expected.

@bbqsrc
Copy link
Owner

bbqsrc commented May 11, 2023

Ah, I didn't realise this would affect incremental builds, that's a solid justification.

Instead of the version check there, we can do it at build time and append it to the generated .exe names, like cc-3.1.1.exe, that way we don't have to do an extra call to get the values, and you can run multiple cargo-ndks without issue.

@bbqsrc
Copy link
Owner

bbqsrc commented May 11, 2023

Or even just name the .cargo-ndk directory with the version, that'd be an even easier workaround. :)

@ScSofts
Copy link
Contributor Author

ScSofts commented May 11, 2023

OK👌

@rib
Copy link
Contributor

rib commented May 11, 2023

It sounds a bit odd to give the directory a '.' prefix. That would make it hidden on other OSs. I think we already established the use of a cargo-ndk subdirectory with a previous workaround for the r23 toolchain, can we not use the same subdirectory that was used by older versions?

@ScSofts
Copy link
Contributor Author

ScSofts commented May 12, 2023

It sounds a bit odd to give the directory a '.' prefix. That would make it hidden on other OSs. I think we already established the use of a cargo-ndk subdirectory with a previous workaround for the r23 toolchain, can we not use the same subdirectory that was used by older versions?

Perhaps the prefix '.' indicates that it is a temporary directory. But actually IDK.

@ScSofts
Copy link
Contributor Author

ScSofts commented May 12, 2023

Or even just name the .cargo-ndk directory with the version, that'd be an even easier workaround. :)

Modification completed. TKS for your suggestion

@bbqsrc bbqsrc merged commit 8330985 into bbqsrc:main May 12, 2023
@bbqsrc
Copy link
Owner

bbqsrc commented May 12, 2023

Made a couple of changes to handle OUT_DIR properly, and is now up as v3.1.2. :)

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 this pull request may close these issues.

4 participants