Split up project in library and binary #21

CohenArthur · 2021-06-22T13:28:28Z

Closes #19
Closes #22
In reference to this comment

@flip1995 I am unsure about the implementation as I usually just panic when handling errors in the main haha. I believe this to be cleaner, as it displays useful information and exits using std::process::exit() rather than panicking.

flip1995 · 2021-06-22T14:22:19Z

Did you look into if using anyhow and/or thiserror would make sense for this?

Using those crates would save you from writing boilerplate code and keeping all that boilerplate code up to date when adding new error sources.

Also a more general note: I saw that you have a main.rs and then a gccrs module. Usually you just make a lib and add one small main.rs to keep this split. You could do this, by moving everything from src/gccrs/* to src/ and rename mod.rs to lib.rs

CohenArthur · 2021-06-22T14:31:23Z

Did you look into if using anyhow and/or thiserror would make sense for this?

Using those crates would save you from writing boilerplate code and keeping all that boilerplate code up to date when adding new error sources.

I didn't look into it and have never used them yet, but I'm open to trying! I think thiserror would be simpler to work with and contribute to.

Also a more general note: I saw that you have a main.rs and then a gccrs module. Usually you just make a lib and add one small main.rs to keep this split. You could do this, by moving everything from src/gccrs/* to src/ and rename mod.rs to lib.rs

Fair point. I didn't know at the time if we were gonna need multiple modules or not. I think it's established now that it won't be necessary, so I'll do this. Maybe in a future PR (I'll open an issue) as to not clutter this one

flip1995 · 2021-06-22T14:43:03Z

You usually want to use thiserror for the library part and then anyhow in the binary part. I think this is a good idea, since you'll get good looking error messages for free.

philberty

LGTM

CohenArthur · 2021-07-04T09:20:03Z

I cleaned up the error handling but this is a temporary and ugly solution. This way, as @flip1995 said, we get good looking error messages and handling for free. Once the project is split into binary and library (#22), this will get refactored. There will be a single error type (not a WrapperError and a gccrs::Error one), and the binary will not need to map errors etc as it does now.

flip1995 · 2021-07-05T10:39:03Z

How much work do you think it would be to do the split between lib and binary before merging this PR? AFAICT this shouldn't be that hard to do. I'd rather do that first and then do this PR right once, instead of merging this PR -> do the split -> merging a PR refactoring this PR.

CohenArthur · 2021-07-05T12:04:09Z

How much work do you think it would be to do the split between lib and binary before merging this PR? AFAICT this shouldn't be that hard to do. I'd rather do that first and then do this PR right once, instead of merging this PR -> do the split -> merging a PR refactoring this PR.

You make a good point haha. I'll do the split here and rename the PR then

flip1995

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

What is this line supposed to do?

cargo-gccrs/src/gccrs.rs

Line 23 in e47fe3b

if cfg!(release) {

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

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)

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?

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.

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.

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 · 2021-07-07T07:56:02Z

src/lib.rs

+
+/// Abstraction around spawning a new `cargo` process with `cargo-gccrs` set as a wrapper
+/// to `rustc`
+pub struct Wrapper;


Why do you need a wrapper for this? Do you plan to do more things with this wrapper in the future? If not, I'd just implement the spawn() function here directly.

That's what I had done originally but thought it'd be cleaner from the binary side of things. I'm fine with reverting to a simple function named spawn_as_wrapper() as it was originally

flip1995 · 2021-07-07T08:05:16Z

src/lib.rs

+        // Skip `cargo` and `gccrs` in the invocation. Since we spawn a new cargo command,
+        // `cargo gccrs arg0 arg1` will become `cargo run arg0 arg1`
+        let mut cargo_gccrs = std::process::Command::new("cargo")
+            .env("RUSTC_WRAPPER", "cargo-gccrs")


I'd rather name this gccrs-driver to stick to the rustc naming.

flip1995 · 2021-07-07T08:11:26Z

src/main.rs

+        Some("gccrs") => Wrapper::spawn(),
+        Some("rustc") => Gccrs::handle_rust_args(&args),


Wait, does this handle both the driver and the cargo part? So if you call cargo gccrs build, this binary will execute itself with the same arguments?

When is this called with gccrs vs rustc?

When you do cargo gccrs, cargo will run cargo-gccrs gccrs. When you do RUSTC_WRAPPER=cargo-gccrs, the spawned rustc processes will be cargo-gccrs rustc. (RUSTC instead of RUSTC_WRAPPER would omit the rustc part)

flip1995 · 2021-07-07T08:53:38Z

Let's track and further talk about this in #36 and revisit this PR once the other points are addressed. Fixing everything in #36 should bring this to a point, where this split can just be done by moving files around.

CohenArthur · 2021-08-16T18:03:28Z

@flip1995 I believe that all items have been resolved. I also took the liberty of splitting the wrapper in two binaries as mentioned in your big review comment, since it ties in nicely with the PR.

philberty

LGTM good work

flip1995

LGTM overall. My 2 comments shouldn't block this PR from going through.

src/bin-driver.rs

flip1995 · 2021-08-23T08:57:57Z

src/bin-driver.rs

+    let args: Vec<String> = std::env::args().collect();
+
+    let res = match args.get(1).map(String::as_str) {
+        Some("rustc") => Gccrs::handle_rust_args(&args),


A function call named handle_rust_args that also compiles things is a bit confusing IMO.

That's fair. Do you have any idea as to what to call the function? Since it doesn't only compile, and also has the task of translating the Rust arguments

I would either split it up and first handle the args and then compile or just call it compile if you want to keep only one public function to interface with the lib part.

flip1995 · 2021-08-24T20:31:34Z

src/gccrs.rs

@@ -136,7 +136,7 @@ impl Gccrs {
    }

    /// Convert arguments given to `rustc` into valid arguments for `gccrs`
-    pub fn handle_rust_args(args: &[String]) -> Result {
+    pub fn compile_rust_args(args: &[String]) -> Result {


Now the method name sounds like it compiles the arguments. Why not just "compile(args)"?

I have a bit of an issue with compile because it sounds like we're simply using some sort of gccrs wrapper, which would logically take gccrs arguments only. I would be okay with compile since the gccrs module will not become its own crate or anything, but I feel weird looking at Gccrs::compile(args) when the args in question are clearly rustc args and not gcc args.

I don't know, I thought this was a good compromise but I'm okay with renaming it as just compile if you'd like

That's a good point. What about the name compile_with_rust_args? So adding a "with". (This is really NIT picking, so if you don't want to change it, but just merge it, I'd also be ok with that)

Perfect! This is what I was thinking too this morning :)

flip1995

Great, let's get this merged then!

CohenArthur requested a review from flip1995 June 22, 2021 13:28

CohenArthur force-pushed the cleanup-error-handling branch from 79dd7e4 to 9fcfe54 Compare June 22, 2021 13:28

CohenArthur mentioned this pull request Jun 22, 2021

Split up sources and create library #22

Closed

philberty approved these changes Jun 24, 2021

View reviewed changes

CohenArthur added 2 commits July 4, 2021 11:16

wrapper: Cleanup error handling and exit gracefully instead of panicking

f978f5b

main: Handle errors using AnyHow before library split cleanup

4dbf5aa

CohenArthur force-pushed the cleanup-error-handling branch from 049dd13 to 4dbf5aa Compare July 4, 2021 09:17

CohenArthur changed the title ~~Add better terminal error handling~~ Split up project in library and binary Jul 6, 2021

flip1995 requested changes Jul 7, 2021

View reviewed changes

flip1995 mentioned this pull request Jul 7, 2021

[Tracking Issue] Whole project review #36

Closed

7 tasks

CohenArthur added 2 commits July 8, 2021 00:11

refactor: Split project in library and binary

a4af01e

refactor: Add gccrs mod and Wrapper type

e47fe3b

CohenArthur force-pushed the cleanup-error-handling branch 2 times, most recently from 488d50f to 58bc1b7 Compare August 16, 2021 17:42

CohenArthur added 2 commits August 16, 2021 19:43

Merge branch 'main' into cleanup-error-handling

58bc1b7

wrapper: Split both binaries in two different files

df08837

CohenArthur force-pushed the cleanup-error-handling branch from 892cefa to 550023c Compare August 16, 2021 18:01

wrapper: Remove Wrapper abstraction

550023c

CohenArthur requested review from philberty and flip1995 August 16, 2021 18:03

CohenArthur mentioned this pull request Aug 21, 2021

Fix CI failure on apt install #52

Merged

Merge branch 'main' into cleanup-error-handling

619727c

philberty approved these changes Aug 21, 2021

View reviewed changes

flip1995 approved these changes Aug 23, 2021

View reviewed changes

bin: Add module documentation regarding both uses of the binaries

dcaabc1

flip1995 reviewed Aug 24, 2021

View reviewed changes

gccrs: Rename handle_rust_args -> compile_with_rust_args

e630c9b

CohenArthur force-pushed the cleanup-error-handling branch from ba209da to e630c9b Compare August 25, 2021 08:05

flip1995 approved these changes Aug 25, 2021

View reviewed changes

Merge branch 'main' into cleanup-error-handling

05314ef

CohenArthur merged commit 87a9173 into main Aug 25, 2021

CohenArthur deleted the cleanup-error-handling branch August 25, 2021 08:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split up project in library and binary #21

Split up project in library and binary #21

CohenArthur commented Jun 22, 2021 •

edited

flip1995 commented Jun 22, 2021

CohenArthur commented Jun 22, 2021

flip1995 commented Jun 22, 2021

philberty left a comment

CohenArthur commented Jul 4, 2021

flip1995 commented Jul 5, 2021

CohenArthur commented Jul 5, 2021

flip1995 left a comment •

edited

flip1995 Jul 7, 2021

CohenArthur Jul 7, 2021

flip1995 Jul 7, 2021

flip1995 Jul 7, 2021

bjorn3 Jul 7, 2021

flip1995 commented Jul 7, 2021

CohenArthur commented Aug 16, 2021

philberty left a comment

flip1995 left a comment

flip1995 Aug 23, 2021

CohenArthur Aug 23, 2021

flip1995 Aug 23, 2021

flip1995 Aug 24, 2021

CohenArthur Aug 24, 2021

flip1995 Aug 25, 2021

flip1995 Aug 25, 2021

CohenArthur Aug 25, 2021

flip1995 left a comment

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

	//! 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.

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

	// 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
	}

		Some("gccrs") => Wrapper::spawn(),
		Some("rustc") => Gccrs::handle_rust_args(&args),

Split up project in library and binary #21

Split up project in library and binary #21

Conversation

CohenArthur commented Jun 22, 2021 • edited

flip1995 commented Jun 22, 2021

CohenArthur commented Jun 22, 2021

flip1995 commented Jun 22, 2021

philberty left a comment

Choose a reason for hiding this comment

CohenArthur commented Jul 4, 2021

flip1995 commented Jul 5, 2021

CohenArthur commented Jul 5, 2021

flip1995 left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flip1995 commented Jul 7, 2021

CohenArthur commented Aug 16, 2021

philberty left a comment

Choose a reason for hiding this comment

flip1995 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flip1995 left a comment

Choose a reason for hiding this comment

CohenArthur commented Jun 22, 2021 •

edited

flip1995 left a comment •

edited