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

feat: use in memory extraction #130

Closed
wants to merge 1 commit into from
Closed

feat: use in memory extraction #130

wants to merge 1 commit into from

Conversation

somehowchris
Copy link
Contributor

@somehowchris somehowchris commented Apr 30, 2022

The dependency tempdir is deprecated and unmaintained since 2018 and got moved into tempfile. But why would this crate need a temporary directory if it know what files its looking for?

I tried to implement this in a way to not to have to copy thing over and over again and work with types which are already returned and used by crates such as reqwest.

Quickly tried it with cargo-watch (provided via repo itself) and cargo-all-features (provided via quickinstall)

Would love to have feedback on this.

Edit: this would also mean that the falg --no-cleanup would be gone as there would be no cleanup, some changes may also be from cargo clippy

@somehowchris somehowchris changed the title feat: use in memory extration feat: use in memory extraction Apr 30, 2022
@ryankurte
Copy link
Collaborator

ryankurte commented Apr 30, 2022

hey thanks for all the PRs!

But why would this crate need a temporary directory if it know what files its looking for?

i can't remember the issue on which it was discussed, but, one of the concepts we had was to improve default behaviours to the point binstall would work for more cases without any configuration.

a planned improvement was globbing through archives to find binaries that match those defined in the Cargo.toml, with some slop for target or version, though maybe this also could be done in memory?

@somehowchris
Copy link
Contributor Author

somehowchris commented May 1, 2022

About the PRs, I love to contribute to project I use, so no worries, just tell me when to backoff 😄

I mean, yes fair thing than can be done. But what Is the point to download, extract, read 1 (or few) files of the archive and delete it again? That data is anyways in memory, so why choose to make the roundtrip to the disk?

@somehowchris somehowchris marked this pull request as draft May 1, 2022 08:06
@somehowchris
Copy link
Contributor Author

I had to do some cross platform debugging to actuall see whats from. Had to change some things to a more functional approach. But pretty much everything is the same.

Tested on my windows, wsl and linux env:

  • cargo binstall cargo-watch --no-confirm
  • cargo binstall cargo-all-features --no-confirm
  • cargo binstall cargo-binstall --manifest-path . --no-confirm

Some of the newly added things like checking if the install directory or the manifest exist came up as it just errored.

I have a macbook at home but will have to wait until later this week to get there, so if someone has one or a arm7 client, feedback would be appriciated

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.

None yet

2 participants