Skip to content
This repository has been archived by the owner on Jun 2, 2024. It is now read-only.

Is the library alive? #232

Closed
zamazan4ik opened this issue Apr 15, 2021 · 29 comments
Closed

Is the library alive? #232

zamazan4ik opened this issue Apr 15, 2021 · 29 comments

Comments

@zamazan4ik
Copy link
Contributor

Hi!

I see that the library has no activity from the developers during the last months: many answered issues and open pull requests without a review. And at least for me, the library seems unmaintained. It's a pity since the library seems the most mature zip implementation in the Rust ecosystem.

I completely understand that developing and maintaining open source libraries is too time-consuming and without any profit.

So I have several questions:

  • Is the library actively maintained? Maybe you as maintainers just had busy months. Or maybe you've lost interest in the project?
  • If you are not anymore interested in the library, can we try to find other maintainers and transit ownership?

Thanks in advance!

@a1phyr
Copy link
Contributor

a1phyr commented Apr 19, 2021

@Plecra you seem to be the only member of the organization that owns the repo. Are you still willing to maintain this crate ?

(Edit: I didn't see you released a new version three days ago)

@BenjaminRi
Copy link

I keep an eye on this library. I implemented the ZipCrypto part, if there's an issue with that, I'm happy to help. I'm also available for general help regarding the library. The reason why I don't take a more active role in development of the library is lack of time on my part and interface design disagreements with the current maintainer(s).

@Plecra
Copy link
Member

Plecra commented Apr 19, 2021

TL;DR With multiple outstanding issues, the crate shouldn't be considered maintained at the moment. I will be working on this in May, and would also be happy to talk to anyone interested in joining as a maintainer.

Hi there! I was actually emailed a few days ago about this topic, and here's an update

You're right, the crates not in great shape at the moment.
Progress is mostly being blocked by the crate's architecture at the moment - most pull requests are adding branches to a select few functions to add in features. Because the logic overlaps, the work to combine the changes is only growing.
I will be refactoring these functions out to ease this problem, but the changes that requires aren't small, and I haven't had the time I hoped for to implement it :)
[...]
I'd also be interested in finding time to merge some of the other PRs as they are. I should have some time in May assuming nothing else springs up on me [...]

Personally, I have been busy nearing the end of my GCSE courses. I would be happy to welcome a co-maintainer, and my only concerns there are with trust. I'm aware of a few security sensitive projects using this crate so I wouldn't want to risk a new maintainer potentially being malicious. Any tips on how to do this safely would be great!

I'd also like to thank @BenjaminRi for the time you've been able to give the crate 😄 Your input is always very helpful. (If you'd like to talk about interface design, please do reach out~)

@Plecra Plecra pinned this issue Apr 19, 2021
@BenjaminRi
Copy link

(If you'd like to talk about interface design, please do reach out~)

One of the interface disagreements is that if you try to unzip an encrypzed ZIP file, but you supply no password, it returns a ZipError::UnsupportedArchive ( https://github.com/zip-rs/zip/blob/master/src/read.rs#L485 ) error. In my opinion, that is the wrong class of error, it is misleading the API user because the archive is actually supported, so it should return a ZipError::PasswordRequired error which is how I originally implemented it in my PR ( https://github.com/BenjaminRi/zip-rs/blob/pkzip-cipher/src/read.rs#L400 ). What is the API user supposed to do to distinguish an actually unsupported archive from a missing password, programmatically? Match the "Password required to decrypt file" error string? It's not even documented. What if you alter the error string because, for example, you think "Password required" is nicer? You silently break the matching code of all your users (it still compiles for them, but no longer matches the different string). Don't make error strings an essential part of the API. These should all be enum values for robust and easy programmatic handling.

I wrote a test for missing password in my original PR ( https://github.com/BenjaminRi/zip-rs/blob/pkzip-cipher/tests/zip_crypto.rs#L50 ) which was indeed changed to match the error string ( https://github.com/zip-rs/zip/blob/master/tests/zip_crypto.rs#L50 ).

However, we've discussed such issues at length in #197 and essentially arrived at an "agree to disagree" conclusion. I feel like serious API issues are not taken seriously and that we should think very carefully about such error values in future releases.

@zamazan4ik
Copy link
Contributor Author

zamazan4ik commented Dec 18, 2021

Any updates on the maintenance status? I see quite a lot of waiting-for-review PRs.

@zamazan4ik
Copy link
Contributor Author

zamazan4ik commented Jan 20, 2022

@Plecra seems like you still have no time to maintain the library. Could you please add me as a co-maintainer to the organization? I can try to help with the maintenance of this library. Thanks!

@zamazan4ik
Copy link
Contributor Author

@mvdnes maybe you can help with providing permissions for the library :)

@Plecra
Copy link
Member

Plecra commented Jan 21, 2022

Hey :) Yup, that's a fair assessment. My apologies.

I can add a maintainer to the project. My main concern with past PRs has been with compromising the crate - I didn't want to make large feature additions without the opportunity to fully review the code, especially since like you said, the crate is used in some fairly prominent projects.. Aiui, mvdnes shares this attitude.

Help with making the changes the crate needs (quite desperately now :P) is super appreciated! For now, I will keep publish access for the crate, however if you can setup a release, I'll happily push it through after a quick check. If you'd like to make direct changes, a third party review would be great. I know I'm probably being overly cautious, but I'd rather not add a maintainer to the crate itself without some trust there ❤️

Would you like me to give you merge access on this repo?

@zamazan4ik
Copy link
Contributor Author

I can add a maintainer to the project. My main concern with past PRs has been with compromising the crate - I didn't want to make large feature additions without the opportunity to fully review the code

Totally agree with you - my request is not related to the past PRs.

I know I'm probably being overly cautious, but I'd rather not add a maintainer to the crate itself without some trust there

I really like such "overly cautious" behavior regarding such libraries - I have the same mindset :)

Would you like me to give you merge access on this repo?

Yes, please. I still will try to push as many as possible changes via PRs (since it's a good way to give a chance to review changes and highlight some decisions). But if for some of my PRs or any external PRs I will not be able to get approval from anyone for some time, merge access will be quite helpful. If you or anyone from "trusted" people will be able to review PRs - it will be really great and helpful!

Regarding new releases - I think it's quite enough to just ping you when the next release for the library will be ready. I hope you will have enough time to release the new library. But if for some reason you will not be able to release the library - please share your release permission with any trusted and responsible person :) My real wish is just not to let this library die since according to the stats it's quite important for the Rust community.

If you have any questions, you can ping me directly.

@Plecra
Copy link
Member

Plecra commented Jan 21, 2022

@zamazan4ik fantastic! I have sent you an invite (oh, you've accepted :))

Releases won't be a problem. I might ask you to bump the version number if the changes feel significant enough from 0.5, whether they're breaking or not.

Contact me if you need anything. I'm probably more responsive on my Discord (Plecra#5251)

@zamazan4ik
Copy link
Contributor Author

zamazan4ik commented Jan 23, 2022

Since I got "maintainer" permissions for the library, I think at least for now users can assume that the library is maintained. I will try at least update dependencies, fix some important issues and work with incoming PRs.

@syphar
Copy link

syphar commented Jan 23, 2022

Thank you @zamazan4ik for pushing things forward :) ( and thank you @Plecra for the trust).

We're using this library inside docs.rs and I'm thankful that things are getting alive again here. I wouldn't want to do another rewrite :)

@jqnatividad
Copy link

Hi @Plecra , @zamazan4ik ,
Is there an ETA to the next patch release, even with just some minor dependency bumps, even before the outstanding PRs are merged?

It's just that time has an open security advisory (which was already addressed in #257), and it'd be great if that can be closed.

https://rustsec.org/advisories/RUSTSEC-2020-0071

Thanks!

@zamazan4ik
Copy link
Contributor Author

@jqnatividad Hi!

I completely understand your worries. Unfortunately, I have no release permissions, so @Plecra is the only one who can do it right now.

As far as I know, he wanted to release a new version soon. For now, I can suggest only depending on the master branch with a specific commit. I cannot do any more for now, sorry.

@jqnatividad
Copy link

Hi @zamazan4ik, I see that you've bumped all the dependencies, most especially time, which takes care of this security advisory.

https://rustsec.org/advisories/RUSTSEC-2020-0071

@Plecra it'd be an awesome time for a new release! 😉

@jqnatividad
Copy link

@zamazan4ik With today's release of 0.6.0 🎉💯 👍 , I think this issue should be closed.

@zamazan4ik
Copy link
Contributor Author

Thanks @Plecra for the release! I don't know, do we want to close the issue? At least before the next frozen period)

@obbardc
Copy link

obbardc commented May 19, 2022

it'd be great to get a new release soonish :-)

@zamazan4ik
Copy link
Contributor Author

@obbardc yeah, you are right. We have a bunch of quite important for our users fixes in the master branch. We will try to release them soon. @Plecra will you be able to publish the new release?

agostbiro added a commit to agostbiro/svm-rs that referenced this issue Sep 5, 2022
The previous version of the `zip` package has an outdated
transitive Rust Crypto dependency that conflicts with newer
Rust Crytpo crates.

The master branch of the `zip` package already contains the new
Rust Crytpo dependencies, but it's [waiting](
zip-rs/zip-old#232 (comment))
for a release for three months now so I've specified the latest
commit in the master branch as the `zip` version.
@NobodyXu
Copy link
Contributor

NobodyXu commented Sep 9, 2022

@zamazan4ik With today's release of 0.6.0 🎉💯 👍 , I think this issue should be closed.

Unfortunately, the updates from this repository is quite slow and @zamazan4ik doesn't have permission to publish new crate and they don't have much time/effort to review PRs.

We still need @Plecra for PR review, but @Plecra also don't have much time for this.

I'd say we need more maintainers and they need to have access to crates.io

@usagi
Copy link

usagi commented Sep 26, 2022

I am having problems with dependency resolution with other major libraries such as actix-web because repos updates are not being published to crates.io. We need to do something about this.

Note: Workaround for Rust newbies who reuiqred zip

If you encounter errors like the following:

$ cargo check
error: failed to select a version for `zstd-sys`.
    ... required by package `zstd-safe v4.1.4+zstd.1.5.2`.
    ... which satisfies dependency `zstd-safe = "=4.1.4"` of package `zstd v0.10.0+zstd.1.5.2`.
    ... which satisfies dependency `zstd = "^0.10.0"` of package `zip v0.6.2`.
    ... which satisfies dependency `zip = "^0.6.2"` (locked to 0.6.2) of package ...
...

Then, in Cargo.toml, change the zip dependency configuration from using crates.io with version to using this repos directly with git:

# zip = "0.6.2"
zip = { git = "https://github.com/zip-rs/zip" }

Or, if you don't need zstd feature, this way it remains available to publish with crates.io. 👍:

# zip = "0.6.2"
zip = { version = "0.6.2", default-features = false, features = [
 "aes-crypto",
 "bzip2",
 "deflate",
 "time",
 # "zstd"
] }

Or, also you can use the new fork version:

binstall-zip = "0.6.3"
// first, use the alias
use binstall_zip as zip;
// and you can use your source code that use `zip` as is.
use zip::{result::ZipResult, write::FileOptions, CompressionMethod, ZipArchive, ZipWriter };

@ssokolow
Copy link

ssokolow commented Sep 26, 2022

That'll make your crate unpublishable. Just turn off zstd support. (default-features = false and then enable the features you care about.)

It's not as if it's very common, given that, last I checked, it wasn't supported by the standard archive extractors that come with Windows or macOS, or by Info-ZIP (the unzip command).

@NobodyXu
Copy link
Contributor

@usagi cargo-binstall also needs zip and what we did is to create a new fork published under a different name so that we can publish our crates.

@usagi
Copy link

usagi commented Sep 26, 2022

@ssokolow @NobodyXu Thanks for the additional infos! I've updated the workaround note of my previous post. 😉

@NobodyXu
Copy link
Contributor

Just note that the fork we used contains an unmerged patch that includes a new API, otherwise it's fine.

We don't intend to add any more features to the fork other than having the unreleased code and the patch.

@Plecra
Copy link
Member

Plecra commented Oct 13, 2022

Hi! I'm not sure I fully understand the problem with resolving dependencies. Correct me if I'm wrong, it seems that there are projects which:

  • depend on zip ^0.6
  • didn't exclude default features originally, because they didn't need to
  • have, one way or another, depended on libzstd through a fixed crate version
  • which is incompatible with the version that zip transitively depends upon in 0.6.2

Cargo generally attempts to resolve conflicts like these, so it's difficult for us to guess at what configuration is causing the problem. If you could link to a repo reproducing this issue (and minimise it if possible! removing third party dependencies) so we can throw up an issue for it, that would be fantastic.

@NobodyXu
Copy link
Contributor

@Plecra zip 0.6 depends on zstd 0.10, which pulls in zstd-sys 1.6.3+zstd.1.5.2.
If you pull in zstd 0.11, then it will also pull in zstd-sys 2.0.1+zstd.1.5.2.

Both versions of zstd-sys pulls in the same version of libzstd, which will produce duplicate symbols and cause linking errors.

@Plecra
Copy link
Member

Plecra commented Oct 13, 2022

great, so it's an update needed here. can do :)

@Plecra
Copy link
Member

Plecra commented Feb 1, 2023

r.e. OP: Yup!

@Plecra Plecra closed this as completed Feb 1, 2023
@Plecra Plecra unpinned this issue Feb 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants