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

fix: Restore original file timestamp when unzipping with chrono #46

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

Pr0methean
Copy link
Member

No description provided.

@Pr0methean Pr0methean changed the title Oldpr436 fix: Restore original file timestamp when unzipping with chrono Apr 30, 2024
@Pr0methean
Copy link
Member Author

@mass10 Please sign your commits.

@mass10
Copy link

mass10 commented May 3, 2024

@Pr0methean Oh,

How can I sign my existing commits ?

(Or should I re-create pull request?)

@Pr0methean
Copy link
Member Author

Pr0methean commented May 3, 2024

You should probably re-create the pull request once you have GPG signing configured. Squashing it into one commit may help, because IIRC some git commands will only sign the last commit in the push. I believe that when you make a commit using GitHub's Web UI and your account has 2FA enabled, GitHub will sign your commits and apply the "Verified" badge without your having to configure GPG.

@mass10
Copy link

mass10 commented May 3, 2024

I am looking at ones with label #please-sign-your-commit ...

@Pr0methean
Copy link
Member Author

You'll only be able to help with ones where you are, or are in contact with, the commit author.

@mass10
Copy link

mass10 commented May 3, 2024

Ok, I see. I will re-create a new pull request signed.

thank you!

@Pr0methean
Copy link
Member Author

If your new PR fails the build_and_test checks and it doesn't seem to be your fault, let me know; I only just committed b6caa1c tonight (to reduce the risk of having one bad PR break the whole merge queue the way #58 did, by not being tested against enough feature configurations), and there's a risk that I may have to add more feature-flag permutations.

@Pr0methean
Copy link
Member Author

Pr0methean commented May 3, 2024

From https://github.com/zip-rs/zip2/actions/runs/8929959279/job/24528958007?pr=46:

 error[E0658]: use of unstable library feature 'file_set_times'
Error:    --> src/read.rs:574:29
    |
574 |                     outfile.set_modified(t)?;
    |                             ^^^^^^^^^^^^
    |
    = note: see issue #98245 <https://github.com/rust-lang/rust/issues/98245> for more information

You may have to use https://crates.io/crates/rustc_version_runtime to guard this statement so that it only gets compiled when building against Rust versions where file_set_times is stable. Skipping the statement on older rustc versions seems safer to me than cfg-guarding a larger block of code.

@Pr0methean
Copy link
Member Author

Pr0methean commented May 3, 2024

Be especially careful around #87, since that seems especially likely to be incompatible with other PRs that come ahead of it in the merge queue. I'd especially look at, and be especially prepared to rebase against, c2c52f2 if I were you.

@Pr0methean
Copy link
Member Author

Pr0methean commented May 3, 2024

@mass10 Also, your oldest 2 commits aren't signed and verified (see https://docs.github.com/authentication/managing-commit-signature-verification/about-commit-signature-verification). Once you set up signing, you may need to squash this PR's commits into one and open a new PR, because some Git commands will only sign the latest commit in the push.

@Pr0methean
Copy link
Member Author

Pr0methean commented May 10, 2024

set_modified and set_times were added in Rust 1.75.0, but the current MSRV is 1.70.0. Please update it in README.md, Cargo.toml and ci.yml. Also, files may have a created time and an accessed time because of #82.

@mass10
Copy link

mass10 commented May 11, 2024

@Pr0methean Please give me a little more time.

Appreciate your great support!

@mass10
Copy link

mass10 commented May 18, 2024

I have found that ZipArchive::extract still doesn't restore original file timestamp on new zip2. (I want extract operation restore the original file timestamp like as unzip on UNIX/LINUX.)

I will try to create a new pull request.

So, this #46 can be closed.

@mass10
Copy link

mass10 commented May 18, 2024

Now I am struggling with rustup 1.7, mmm.........

@Pr0methean Pr0methean added waiting to be automatically merged If this PR isn't automatically merged, suspect a broader issue. and removed waiting to be automatically merged If this PR isn't automatically merged, suspect a broader issue. labels Jul 6, 2024
@Pr0methean
Copy link
Member Author

@mass10 My policies for this crate have changed: I no longer require commit signing, and I'm willing to increase the MSRV to 1.75. Are you still interested in finishing this PR?

@mass10
Copy link

mass10 commented Jul 7, 2024

@Pr0methean Oh, Yes! I think we need this PR.

@Pr0methean
Copy link
Member Author

From the failing unit test, it looks like you're missing [cfg(feature = "chrono")] on some of the new code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants