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

Rebasing on Rust3DS repo #8

Closed
Meziu opened this issue Jan 5, 2022 · 15 comments
Closed

Rebasing on Rust3DS repo #8

Meziu opened this issue Jan 5, 2022 · 15 comments

Comments

@Meziu
Copy link
Member

Meziu commented Jan 5, 2022

@AzureMarker I rebased ctru-rs on your AzureMarker/ctru-rs. I split your repo in 2 crates:

ctru-sys - Raw bindings for libctru
ctru-rs - Safe layer over ctru-sys

I understand very well your Cargo Workspace approach, but in the view of (eventually and hopefully) release the crate into crates.io, the separate approach works best. This is valid for a couple of reasons:

  1. ctru-sys has to update WAY less then ctru-rs, and carrying both is wasteful on Cargo's part
  2. Since many features from libctru aren't going to be (possibly ever) layered by ctru-rs, some people may want to use and work with only ctru-sys.
  3. It's easier to keep track of changes in ctru-rs, and the tree is less heavy.

I hope you will accept these changes and we will start working on the same tree. 😄
If you have any issues to note, please do let me know.

@AzureMarker
Copy link
Member

Releasing a crate happens individually, not per repository. So it's better to keep them in the same repo.

@AzureMarker
Copy link
Member

The cargo workspace will only be relevant for contributors to the repo. From the point of view of users and crates.io, the crates are just related by dependency.

@AzureMarker
Copy link
Member

The crates are very closely tied together anyways, so having them in separate repos would make it harder to see how a change in sys affects ctru-rs. I highly encourage moving the crates back into one repo like my fork has it.

@Meziu
Copy link
Member Author

Meziu commented Jan 5, 2022

Yes, crates are indeed uploaded separately, but ctru-rs now depends on ctru-sys via path. This makes hard to update ctru-sys while NOT updating ctru-rs (for any possible contrasts), thing that would instead be handled by crates.io versioning (or even just by using a commit hash with git). They are indeed close together, but ctru-rs is completely unnecessary and lacks many features if you already have experience with libctru in C. Having them together serves little purpose, and (as it is right now with git) ctru-sys DOES get repulled every time you update ctru-rs. I don't understand why anyone should pull 13 thousands lines of additional code every time a small update releases in ctru-rs, this does affect the end user, not just the contributors.

@AzureMarker
Copy link
Member

AzureMarker commented Jan 5, 2022

You can have a dependency specified with both a path and a version/git repo:
https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#multiple-locations

@AzureMarker
Copy link
Member

AzureMarker commented Jan 5, 2022

@AzureMarker
Copy link
Member

AzureMarker commented Jan 5, 2022

I don't understand why anyone should pull 13 thousands lines of additional code every time a small update releases in ctru-rs, this does affect the end user, not just the contributors.

This is not the case. ctru-rs would depend on a version of ctru-sys. An update to ctru-rs only updates ctru-rs. In crates.io the package data only contains ctru-rs data, not ctru-sys. Updating to a new ctru-rs version will only pull ctru-rs so long as the existing ctru-sys version still satisfies the dependency.

@AzureMarker
Copy link
Member

Publishing a crate only publishes that one crate's data:
https://doc.rust-lang.org/cargo/reference/publishing.html#packaging-a-crate

@AzureMarker
Copy link
Member

I can open a PR to show how this works. It might be easier to have something concrete to talk around.

@Meziu
Copy link
Member Author

Meziu commented Jan 5, 2022

I don't understand why anyone should pull 13 thousands lines of additional code every time a small update releases in ctru-rs, this does affect the end user, not just the contributors.

This is not the case. ctru-rs would depend on a version of ctru-sys. An update to ctru-rs only updates ctru-rs. In crates.io the package data only contains ctru-rs data, not ctru-sys. Updating to a new ctru-rs version will only pull ctru-rs so long as the existing ctru-sys version still satisfies the dependency.

Yes, I know this is how crates.io works. I was talking about using the crate through git. Cargo pulls the specified repository into its cache, and indexes it as one object. Requesting a cargo update once the repo is updated, will result in a full re-download.

@Meziu
Copy link
Member Author

Meziu commented Jan 5, 2022

Still, it's not really important. Your way works anyways.

@AzureMarker
Copy link
Member

AzureMarker commented Jan 5, 2022

Requesting a cargo update once the repo is updated, will result in a full re-download.

While we're still using git dependencies it may be a little inefficient, sure. But updating a git dependency should just be a delta between the old and new versions (like a git pull), which wouldn't mean redownloading ctru-sys. Either way though it's not an issue once we publish to crates.io.

@Meziu
Copy link
Member Author

Meziu commented Jan 5, 2022

If we publish to crates.io. Not to be pessimistic, but 3 months without real news on that libc PR... is there any way to at least receive an answer?

@AzureMarker
Copy link
Member

AzureMarker commented Jan 5, 2022

I expect it will merge eventually. I've had a PR open for months without news, but is now very close to merging: tokio-rs/tokio#3370

We can bump/ping if we don't get news for a while.

@AzureMarker
Copy link
Member

AzureMarker commented Jan 6, 2022

I took a look at how you did the change and the easiest thing to do is just revert/drop the commits you added after forking, and then reapply any small changes you wanted to make (ex. removing travis config, though it should be replaced with something else like GitHub Actions). The Cargo.toml of ctru-rs already refers to ctru-sys via path and version. I think the examples should be kept though. I'm actually planning on testing those out soon.

@Meziu Meziu closed this as completed Jan 6, 2022
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

No branches or pull requests

2 participants