[Tracking Issue] Whole project review

[flip1995]

This is a review of the binary library split branch: cleanup-error-handling

First posted in #21 (review), but I wanted to also track this outside of the PR.

I figured that this is now the right time to do a whole project review, and not only a review of #21. My suggestions don't have to be implemented all in one PR, but rather get implemented step by step. I tried to sort them from easy to hard to do.

---

• cfg!(release)

What is this line supposed to do?

cargo-gccrs/src/gccrs.rs

Line 23 in e47fe3b

if cfg!(release) {

---

• is_installed

cargo-gccrs/src/gccrs.rs

Lines 43 to 44 in e47fe3b

	// On UNIX, we can check using the `command` built-in command, but it's not cross-platform.
	// The slow but sure way to do this is to just try and spawn a `gccrs` process.

You can use the which crate for cross-platform support for this

---

• serde for config dump/parsing?

cargo-gccrs/src/config.rs

Lines 1 to 4 in e47fe3b

	//! The Config module aims to parse gccrs target options and translate them to an output
	//! similar to what rustc does. This module parses a file previously created by the
	//! [`Gccrs::dump_config()`] function. This corresponds to invoking gccrs with the
	//! `-frust-dump-target_options` argument.

What format is the config dump in and can you use serde for this?

---

• Config dump/parsing impl

cargo-gccrs/src/config.rs

Lines 101 to 102 in e47fe3b

	/// Display the gccrs target options on stdout, in a format that cargo understands
	pub fn display() -> Result {

A fallible display impl seems weird to me. You should rather implement a fallible constructor and then the Display trait. That way you can use the format machinery and get functions like to_string for free.

I'm also not 100% sure what the option dumping is for, so please tell me if there is a reason why it has to be done that way.

---

• multiple binaries?

I think you want to have 2 binaries here. One for the driver, that does all the argument conversion and other hard work, and one that just invokes the driver from the cargo command. But I may be misunderstanding something, see my comment #21 (comment)

---

• GccrsArgs not holding all arguments

cargo-gccrs/src/args.rs

Line 171 in e47fe3b

pub fn from_rustc_args(rustc_args: &[String]) -> Result<Vec<GccrsArgs>> {

I still don't quite get why it is necessary to return a Vec<GccrsArgs>. Just from the struct name and the documentation, I would assume that when I have a GccrsArgs instance, that I have all the arguments that I need. This is also the main thing that prevents the GccrsArgs from implementing things like the TryFrom trait and so on.

---

• GccrsArgs is doing too much

cargo-gccrs/src/args.rs

Lines 164 to 168 in e47fe3b

	// Execute an argument set's callback if present. Returns Ok if no callback was
	// present
	pub fn callback(&self) -> Option<&'static dyn Fn(&GccrsArgs) -> Result> {
	self.callback
	}

I still think that with this, GccrsArgs is doing too much. One good principle of OOP is that one class should only do one thing. With that principle, GccrsArgs should only handle arguments, which includes parsing, converting, and then holding them. Now it handles arguments and does random other things like creating archives and in the future possibly even more.

This is the area of the current implementation that really needs a refactor IMHO. Apply the KISS principle as often as you can. I have trouble following what is done where, so the current impl isn't really simple to follow.

[flip1995]

I would address this in the following order:

1. Let's get the good parts of cleanup-error-handling merged first. Open a PR, that just adds thiserror to the library part (that is still kept as a module) and the binary part just calls .expect for now. Don't split lib/binary yet and don't use anyhow for now.
2. Address the simple points 1-4
3. Address point 5
4. Address point 6-7

[CohenArthur]

* [ ]  `cfg!(release)`

What is this line supposed to do?

Since gccrs cannot output some required information yet, we are faking it. Because I do not believe that this is valid solution, I've added this check to prevent the release build (and therefore what someone looking at crates.io might install) from working properly as it is not in a state to do so yet. For development builds, we get the fake output and can keep going. I am also okay with removing it, but thought it would be nice to mark that this is not the actual expected behavior.

* [ ]  `is_installed`

cargo-gccrs/src/gccrs.rs

Lines 43 to 44 in e47fe3b

	// On UNIX, we can check using the `command` built-in command, but it's not cross-platform.
	// The slow but sure way to do this is to just try and spawn a `gccrs` process.

You can use the which crate for cross-platform support for this

Aaah, perfect! Thanks, I'll open an issue for this.

* [ ]  `serde` for config dump/parsing?

cargo-gccrs/src/config.rs

Lines 1 to 4 in e47fe3b

	//! The Config module aims to parse gccrs target options and translate them to an output
	//! similar to what rustc does. This module parses a file previously created by the
	//! [`Gccrs::dump_config()`] function. This corresponds to invoking gccrs with the
	//! `-frust-dump-target_options` argument.

What format is the config dump in and can you use serde for this?

I do not know if the config dump has a specific format. @philberty?
I've figured out the simple format used, but am not sure about how serializable it is. The current "parser" is very simple. I have never used a "new" format with serde so I wouldn't know how to proceed, but I am not against it.

* [ ]  Config dump/parsing impl

cargo-gccrs/src/config.rs

Lines 101 to 102 in e47fe3b

	/// Display the gccrs target options on stdout, in a format that cargo understands
	pub fn display() -> Result {

A fallible display impl seems weird to me. You should rather implement a fallible constructor and then the Display trait. That way you can use the format machinery and get functions like to_string for free.

You're right. Thanks!

I'm also not 100% sure what the option dumping is for, so please tell me if there is a reason why it has to be done that way.

When first launching rustc, cargo asks the compiler to print its configuration as well as a few other options such as the expected name of libraries, binaries etc. This was the first "milestone" to get through, you can find the expected output and associated issue here

* [ ]  multiple binaries?

I think you want to have 2 binaries here. One for the driver, that does all the argument conversion and other hard work, and one that just invokes the driver from the cargo command. But I may be misunderstanding something, see my comment #21 (comment)

I think it would be fine to have two binaries. Basically, what the current one is doing is the following:

• If launched as cargo gccrs [build|run|test|...], spawn as a wrapper around rustc which will create the process cargo-gccrs rustc ...
• If lauched as cargo-gccrs rustc, act as a translator and produce gccrs options from rustc options.

Note the difference in the call with the extra dash. One invokes the cargo subcommand, while the other one invokes the actual binary.

(Oh, @bjorn3 had already answered: #21 (comment) Sorry!)

In order to differentiate, we can simply check the first argument. I agree that having multiple binaries make sense, and I think it's a better way to go about it and it's clearer.

* [ ]  `GccrsArgs` not holding all arguments

cargo-gccrs/src/args.rs

Line 171 in e47fe3b

pub fn from_rustc_args(rustc_args: &[String]) -> Result<Vec<GccrsArgs>> {

I still don't quite get why it is necessary to return a Vec<GccrsArgs>. Just from the struct name and the documentation, I would assume that when I have a GccrsArgs instance, that I have all the arguments that I need. This is also the main thing that prevents the GccrsArgs from implementing things like the TryFrom trait and so on.

For a single rustc invocation, we might need multiple gccrs invocation. For example, rustc --crate-type=bin --crate-type=dyn --crate-type=static file.rs would create a binary, a shared executable and a static library. This behavior is not reproducible in gcc as the compiler can only generate a single output file (as far as I know), so we need to call gccrs three times. Once for the binary, once for the shared library and once for the static library. Therefore, we need three different sets of arguments...

Note that this might change thanks to @bjorn3 's really nice and detailed proposition here that we must discuss.

* [ ]  `GccrsArgs` is doing too much

cargo-gccrs/src/args.rs

Lines 164 to 168 in e47fe3b

	// Execute an argument set's callback if present. Returns Ok if no callback was
	// present
	pub fn callback(&self) -> Option<&'static dyn Fn(&GccrsArgs) -> Result> {
	self.callback
	}

I still think that with this, GccrsArgs is doing too much. One good principle of OOP is that one class should only do one thing. With that principle, GccrsArgs should only handle arguments, which includes parsing, converting, and then holding them. Now it handles arguments and does random other things like creating archives and in the future possibly even more.
This is the area of the current implementation that really needs a refactor IMHO. Apply the KISS principle as often as you can. I have trouble following what is done where, so the current impl isn't really simple to follow.

Good point. I'll get to the refactoring of GccrsArgs really soon! I want to get most of the test harness done before I get on that, and then I'll work on making the code as good as it can be.

I agree that the impl is getting way too big and hard to follow. I haven't worked on it since adding the callback, and it's in desperate need of a refactoring.

[bjorn3]

cfg!(release) is always false unless you pass --cfg release to rustc. There is debug_assertions, but no general way to cfg code out depending on the cargo profile.

[CohenArthur]

cfg!(release) is always false unless you pass --cfg release to rustc. There is debug_assertions, but no general way to cfg code out depending on the cargo profile.

Woops :( Well then I'll remove it. It made sense in theory haha

[flip1995]

Finished in #21

Great work!