search

hlorenzi / customasm

💻 An assembler for custom, user-defined instruction sets! https://hlorenzi.github.io/customasm/web/
Apache License 2.0
719 stars 56 forks source link

Split CLI into its own crate #98

Open parasyte opened 3 years ago

parasyte commented 3 years ago
parasyte commented 3 years ago

@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.

hlorenzi commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

The challenge is that Cargo doesn't have great support for crates that combine a binary with a reusable library. See https://github.com/rust-lang/cargo/issues/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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 https://github.com/rust-lang/cargo/issues/6313 seems relevant.

hlorenzi commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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!