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

Feature: Create metafile format for cargo-binstall #252

Merged
merged 44 commits into from
Jul 28, 2022
Merged

Feature: Create metafile format for cargo-binstall #252

merged 44 commits into from
Jul 28, 2022

Conversation

NobodyXu
Copy link
Member

Implement #176 (comment)

Signed-off-by: Jiahao XU [email protected]

NobodyXu added 30 commits July 26, 2022 21:31
Signed-off-by: Jiahao XU <[email protected]>
Use it to represent source type instead of using `CompactString`.

Signed-off-by: Jiahao XU <[email protected]>
so that we can use `&str` to query `BTreeSet<MetaData>`

Signed-off-by: Jiahao XU <[email protected]>
for `binstall_v1::Records`

Signed-off-by: Jiahao XU <[email protected]>
Signed-off-by: Jiahao XU <[email protected]>
Signed-off-by: Jiahao XU <[email protected]>
@NobodyXu NobodyXu added the PR: feature work PR that provides or works on a new feature label Jul 27, 2022
@NobodyXu
Copy link
Member Author

@passcod Answer to #176 (comment) :

define what the goals for the metafile are. what problems does it solve, what the use will be now and in future

The goals for this PR is to update crates installed by cargo-binstall easily using sth like cargo-tools update -a and for cargo-tools to display currently installed crates and their metadata.

explore a bit the interaction between the metafile and the theoretical manifest+lock described here. are they complementary? conflicting?

If the metafile in this PR is used for global installation and the one in #176 is used for local project only, then I think it is fine.

For local project, you have to install the binary locally too (e.g. $PROJECT/bin and add that to .gitignore) to avoid conflicts anyway.

consider the external use. if an auxillary project like cargo-update wants to use it, could they? if a project like quickinstall, which has its own cli, wants to use it, could they? is that something we want to enable or to discourage?

They definitely can, I think the format is simple enough.
We could also split this into an external crate under a new cargo-binstall org to make it easier for them.

Though for quickinstall it would be easier to just use cargo-binstall and cargo-update probably won't support it for now.

@NobodyXu
Copy link
Member Author

@passcod Answer to #176 (comment) :

I also kinda want to avoid the proliferation of many different files that we write to. ...or maybe we say, let's go and write as many files as we want to, but I'd say let's make our own config folder for all this. there's other stuff we might want to start doing, like keeping caches e.g. for hsts. and having an actual config file, I think that came up once or twice recently

Yep!

I would do that.

@passcod
Copy link
Member

passcod commented Jul 28, 2022

Sounds good. I think the plan is then to:

  • leave the manifest stuff for now for future design refinement
  • your PR and format as-is is good for purpose i think
    • this would be newline separated json, right?
  • decide on a name for a new config folder that's put in all the right OS places, I think the dirs crate has those? mostly this is deciding between either binstall or cargo-binstall
    • though in the latter case we could also put it under $CARGO_HOME/.binstall/...? I don't know if that would be a good idea or a bad idea haha
    • the actual work on this can be another PR and just leave a placeholder in this one, if that makes it clearer

@NobodyXu
Copy link
Member Author

NobodyXu commented Jul 28, 2022

this would be newline separated json, right?

Not necessary newline, but simply separate json objects.
When using jq to print it in pretty mode, the output is separated by newline.

I think the dirs crate has those?

Yeah, dirs::config_dir gives us the dir for user configs.

though in the latter case we could also put it under $CARGO_HOME/.binstall/...? I don't know if that would be a good idea or a bad idea haha

This also sounds ok.

I was thinking about $CARGO_HOME/binstall.

the actual work on this can be another PR and just leave a placeholder in this one, if that makes it clearer

Asides from having 1 or 2 more commits, it should be good.

NobodyXu added 3 commits July 28, 2022 19:43
by using `tuple_vec_map` to make the `Vec<(CompactString,
serde_json::Value)>` appears as a map.

This is OK because we don't access that part anyway and only add such
field to avoid losing any information when deserialize, modify it and
serialize it again to overwrite existing metafile.

Signed-off-by: Jiahao XU <[email protected]>
The postfix should be `.json`, not `.toml`.

Signed-off-by: Jiahao XU <[email protected]>
@NobodyXu
Copy link
Member Author

@passcod I ended up picking $CARGO_HOME/binstall/crates-v1.json because by default, we install the binary to $CARGO_HOME/bin, so I think it is more reasonable to put the dir under $CARGO_HOME.

@NobodyXu NobodyXu marked this pull request as ready for review July 28, 2022 10:01
@NobodyXu NobodyXu requested a review from passcod July 28, 2022 10:01
Copy link
Member

@passcod passcod left a comment

Choose a reason for hiding this comment

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

a few suggestions but looks good otherwise

src/metafiles/cvs.rs Show resolved Hide resolved
src/metafiles/cvs.rs Outdated Show resolved Hide resolved
src/metafiles/binstall_v1.rs Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
@NobodyXu NobodyXu requested a review from passcod July 28, 2022 13:25
@NobodyXu NobodyXu merged commit 0c76185 into cargo-bins:main Jul 28, 2022
@NobodyXu NobodyXu deleted the feature/binstall-format branch July 28, 2022 14:47
@passcod passcod mentioned this pull request Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: feature work PR that provides or works on a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants