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

Split CLI into its own crate #98

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

parasyte
Copy link

@parasyte parasyte commented Aug 5, 2021

  • This allows using the library without depending on the getopts and vergen trees

@parasyte
Copy link
Author

@hlorenzi I think this PR is ready for a review. It would be nice to eventually add lints to CI (clippy, rustdoc, and rustfmt) so that editors with integrated clippy and rustfmt aren't a huge pain to use on this repo.

I did my best to keep the original formatting that is very unfriendly to rustfmt, but I might have messed something up along the way.

- This allows using the library without depending on the `getopts` and `vergen` trees
- Rename executable
- Update CI for cargo workspace
- Update build scripts in root for workspace compatibility
- Always run CI
@hlorenzi
Copy link
Owner

I just got a bit of time to review this, and I think it's a great idea! I'm all for it!

But I'm thinking this might mess up the association with crates.io, especially when people do cargo install customasm. Wouldn't this cause the binary's name to change? Then people wouldn't be able to directly call customasm main.asm and such from the terminal.

I'm also not too fond of the new sub-crate structure. I must admit I'm unfamiliar with Cargo workspaces, so I can't really anticipate whether any issues could arise in the future.

Isn't there a way we could achieve the same effect with features and optional dependencies, without splitting up the crate?

@parasyte
Copy link
Author

The binary name does not change. I don't think it will mess up cargo install ... But I've been wrong before. 😅

I use workspaces quite frequently. It takes getting used to, but it makes managing multiple crates so much nicer in the end. For example, a workspace is a good way to reduce depencency clutter when you have a long list of examples that each have their own set of deps. This is the structure I used for https://github.com/parasyte/pixels (instead of Cargo's built-in example support).

Features are ok, but you don't want to abuse them. You will run into some very frustrating limitations with how Cargo resolves dependencies and other annoyances.

@hlorenzi
Copy link
Owner

hlorenzi commented Aug 28, 2021

Hmm, tried doing a test with https://crates.io/crates/workspace_test

But then when doing cargo install workspace_test, it says:

image

Also, it became cumbersome to run the app during development, since cargo run doesn't work anymore. (You need to use the full cargo run --package workspace_test_cli call)

Oh, and another thing, crates.io doesn't reflect the categories listed in any sub-crates, so we lose a bit of exposure:

image

I think it might be worth investigating the use of features for this, even if not ideal. What are your thoughts on all of this?

@parasyte
Copy link
Author

The challenge is that Cargo doesn't have great support for crates that combine a binary with a reusable library. See rust-lang/cargo#1982 for an example of an issue that this combination has. Personally I don't have a use for the customasm binary, but I would like to use the library.

The options for this are (AFAIK):

  1. Separate crates for the bin and lib (e.g. as in this PR) ... Either can be promoted as the "preferred crate". For instance focus more on the core functionality of the lib, or focus more on the cli aspect of the bin. But not both. In other words, which one do you want to be named customasm? The other needs a different name like customasm-cli or customasm-lib. (Or maybe something less boring.)
    • It makes more sense (to me) that the core functionality is more important than a CLI, but the CLI is still generally a useful tool. That is my rationalization for using this approach for the PR.
  2. Cargo features. This adds the requirement that dependents may have to disable default features to exclude unneeded parts (like cli stuff). This can be a good option when there are few features and they don't cause conflicts or build errors with certain configurations.

I'm open to either option. Be aware of the tradeoffs and what you anticipate this crate being used for. It comes down to a fundamental question; Is customasm a CLI app, or is it a library for building assemblers?

@hlorenzi
Copy link
Owner

Wow, what a complete mess without any resolution, unfortunately. I read that entire thread and all related threads as well. To think we just needed something like a [bin-dependencies] field in the manifest...

When I started this project some 4 or 5 years ago, I think it was actually advisable to have your library and binary in the same crate? I was following the structure suggested by the official book or something.

It makes more sense (to me) that the core functionality is more important than a CLI

I agree with this. Then again, I wasn't expecting many people would want to use the library by itself. That's very interesting!

I think I'll have to do some experiments on my workspace_test crate to be able to judge all the options a little better.

@parasyte
Copy link
Author

When you get a sense of what you want to do here, feel feee to either close this PR with a better one, or you can even take over this and make any changes that better meet your needs.

@parasyte
Copy link
Author

parasyte commented Sep 4, 2021

FWIW, I got this warning message when I updated customasm today.

  Installing customasm v0.11.10
warning: output filename collision.
The bin target `customasm` in package `customasm v0.11.10` has the same output filename as the lib target `customasm` in package `customasm v0.11.10`.
Colliding filename is: C:\Users\jay\AppData\Local\Temp\cargo-installOJ4TIq\release\customasm.pdb
The targets should have unique names.
Consider changing their names to be unique or compiling them separately.
This may become a hard error in the future; see <https://github.com/rust-lang/cargo/issues/6313>.

The linked issue rust-lang/cargo#6313 seems relevant.

@hlorenzi
Copy link
Owner

hlorenzi commented Sep 4, 2021

Oh, no! 😂 Seems like the Cargo team is also pushing for me to split up the crates...

I'm more inclined now to make the library a sub-crate, letting the binary stay as the main one. Is it possible for you to declare a dependency on a sub-crate from crates.io?

@parasyte
Copy link
Author

parasyte commented Sep 4, 2021

Is it possible for you to declare a dependency on a sub-crate from crates.io?

Yes, both crates have to be published separately, but it is otherwise fine to do so.

@hlorenzi
Copy link
Owner

hlorenzi commented Sep 4, 2021

Yes, both crates have to be published separately, but it is otherwise fine to do so.

Oh, you gotta publish them separately...? I wonder if it's common to do that from a single Git repo?

@parasyte
Copy link
Author

parasyte commented Sep 4, 2021

It is quite common! Have a look at https://github.com/nical/lyon/tree/master/crates or https://github.com/tauri-apps/tauri/tree/dev/core and compare with how many crates they have published. These aren't even the biggest examples, just the ones I could think of off the top of my head!

@hlorenzi hlorenzi force-pushed the main branch 2 times, most recently from 93b575d to a82e8a1 Compare June 4, 2023 20:29
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.

2 participants