Skip to content

Check if the universal library is updated before running lipo#42

Open
branchseer wants to merge 3 commits intoTimNN:masterfrom
branchseer:lipo_mtime
Open

Check if the universal library is updated before running lipo#42
branchseer wants to merge 3 commits intoTimNN:masterfrom
branchseer:lipo_mtime

Conversation

@branchseer
Copy link
Copy Markdown
Contributor

This PR checks if the universal library is updated by comparing the mtime of it to the inputs.

If I am working on the non-rust part of an Xcode project, everytime I hit the run button cargo-lipo runs. In this case I expect the cargo-lipo to finish quickly since everything in rust is updated, but it takes noticeable time, because of the lipo command. This PR solves this problem by doing a Makefile-style check before running lipo.

Copy link
Copy Markdown
Owner

@TimNN TimNN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution! Sorry it took a while for me to get to reviewing it.

I've left some review comments, but in general this looks like a good idea. (I'll also look into getting the CI working again, which would have at least caught the formatting issues).

Comment thread src/lipo.rs Outdated
Comment thread src/lipo.rs
Comment thread src/lipo.rs Outdated
Comment thread src/lipo.rs Outdated
Comment thread src/lipo.rs Outdated
use std::path::Path;

fn is_output_updated(output: impl AsRef<Path>, inputs: impl IntoIterator<Item=impl AsRef<Path>>) -> std::io::Result<bool> {
let output_mtime = fs::metadata(output)?.modified()?;
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we handle the "file not found" case here explicitly (by returning that the output needs to be updated)? Then we can also log a warning below if this function fails.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment still applies: Can you please explicitly check if fs::metadata returned "not found"?

Copy link
Copy Markdown
Owner

@TimNN TimNN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for applying the changes, and sorry again for taking so long to review, things have been busy 😒.

Can you please rebase the branch on master? I've fixed the CI setup to run on Github actions now.

Optionally: It would be great to have an integration test for this. If you are interested, consider adding one in integration.bats.sh, otherwise let me know and I will add one.

Comment thread src/lipo.rs Outdated
use std::path::Path;

fn is_output_updated(output: impl AsRef<Path>, inputs: impl IntoIterator<Item=impl AsRef<Path>>) -> std::io::Result<bool> {
let output_mtime = fs::metadata(output)?.modified()?;
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment still applies: Can you please explicitly check if fs::metadata returned "not found"?

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.

3 participants