Compile basic rust project #5

CohenArthur · 2021-06-10T19:37:01Z

This PR allows the compilation of basic, single file Rust projects. I'd love to get some feedback on the different options given to gccrs, the ones I'm parsing from rustc and the implementation's quality overall. There is still a lot of room for improvement but this happily compiles the included basic Rust test. I believe that some necessary improvements are outside the scope of this PR, such as the -C option parser. I will definitely work on that and fix it.

Close #3
Close #4

bjorn3 · 2021-06-10T19:41:55Z

src/gccrs/args.rs

+        match s {
+            "bin" => CrateType::Bin,
+            // FIXME: Is 'lib' really part of this? Not always, right?
+            "dylib" | "lib" => CrateType::DyLib,


lib is an alias for rlib atm. it is officially documented as changable to dylib in the future, but I would consider it highly unlikely because it would break things.

Thanks, will fix!

bjorn3 · 2021-06-10T19:46:07Z

src/gccrs/mod.rs

+            // If rustc is invoked with stdin as input, then it's simply to print the
+            // configuration option in our case, since we are compiling a rust project
+            // with files and crates
+            Some("-") => Gccrs::cfg_print(),


- indicates read from stdin. When you use --print it is just necessary to provide an input file.

My question regarding the FIXME was related to cargo's usage of the "-" argument: Am I right in assuming that it will only ask rustc to read from stdin when initially printing the configuration? There is no weird, hidden flag to compile from stdin that I'm unaware about?

I don't think cargo will ever ask rustc to read from stdin except for when using --print or maybe if using something weird like [lib] path = "-" in Cargo.toml. It os still cleaner to check for the right --print instead though.

I tried it in an empty project, and cargo doesn't build anything with [lib] path = "-". It's invoking rustc/gccrs just like during compilation, which is without redirecting stdin to the spawned process's stdin.

I agree with you that it would be cleaner, but I think it should wait for now. Doing it this way is really practical since we can generate the gccrs.target_options.dump and parse it. Using the GccrsArgs struct and converting --print=cfg into -frust-dump-target_options would be nice, but it would require some sort of callback or other means to implement the parsing and printing of the configuration. Or, as discussed with @philberty, we could also make it so that gccrs displays target options in the same way as rustc, and on stdout instead of in a file. I've opened #7 to remember to get to this at some point

flip1995

Parts of this isn't really idiomatic Rust. So I suggest to use Clippy, which will help you in this regard.

I also see, that you don't really do error handling right now. I recommend that you think about this early. Retrofitting error handling usually isn't an easy task.

.github/workflows/build_test_fmt.yml

Cargo.toml

src/gccrs/args.rs

src/gccrs/mod.rs

src/main.rs

philberty · 2021-06-11T10:26:25Z

Thanks for reviewing @flip1995 and @bjorn3 and good work for this @CohenArthur. I think the review comments here are really good.

There are 2 bigger issues after reading the comments:

Setup Clippy Automation
Error handling

If you prefer you could create github-issues out of these from the relevant comment, this way the other comments can be resolved with subsequent commits to this PR and this can be merged. Or you can continue to keep this PR open to fix those issues its up to you.

CohenArthur · 2021-06-11T23:59:57Z

Thanks for reviewing @flip1995 and @bjorn3 and good work for this @CohenArthur. I think the review comments here are really good.

There are 2 bigger issues after reading the comments:
1. Setup Clippy Automation

2. Error handling
If you prefer you could create github-issues out of these from the relevant comment, this way the other comments can be resolved with subsequent commits to this PR and this can be merged. Or you can continue to keep this PR open to fix those issues its up to you.

I think the error handling and automation are really important and should therefore be part of the first PR in order to set a good base. But I agree that some things are out of scope for this and should be implemented later, as this PR only represents a minimum example of the final solution

flip1995

This PR looks way better/cleaner now, great work!

One more thing regarding GHA.

.github/workflows/build_test_fmt.yml

flip1995

LGTM

.gitignore

flip1995 · 2021-06-14T09:02:13Z

Since inline-comments easily get lost, see #5 (comment)

bjorn3 · 2021-06-14T09:32:08Z

src/gccrs/mod.rs

@@ -61,8 +71,7 @@ impl Gccrs {
        }
    }

-    /// Convert arguments given to `rustc` into valid arguments for `gccrs`
-    pub fn handle_rust_args() -> Result {
+    fn cfg_print() -> Result {


Can you add a fixme that the output needs to be adapted for the target triple?

Good catch, thanks. Done in 98688f7

Co-authored-by: bjorn3 <bjorn3@users.noreply.github.com>

Co-authored-by: flip1995 <hello@philkrones.com>

Co-authored-by: bjorn3 <bjorn3@users.noreply.github.com>

CohenArthur requested review from philberty and flip1995 June 10, 2021 19:37

bjorn3 reviewed Jun 10, 2021

View reviewed changes

flip1995 requested changes Jun 11, 2021

View reviewed changes

philberty added this to In progress in Compile basic rust project via automation Jun 11, 2021

philberty added the enhancement New feature or request label Jun 11, 2021

CohenArthur mentioned this pull request Jun 11, 2021

Parse config options using getopts as well #7

Open

CohenArthur requested a review from flip1995 June 11, 2021 21:59

flip1995 reviewed Jun 14, 2021

View reviewed changes

.github/workflows/build_test_fmt.yml Show resolved Hide resolved

CohenArthur force-pushed the compile-basic-hw branch from 990a7d4 to 8395fe7 Compare June 14, 2021 07:44

flip1995 approved these changes Jun 14, 2021

View reviewed changes

CohenArthur force-pushed the compile-basic-hw branch from 8395fe7 to 71fd09b Compare June 14, 2021 07:57

bjorn3 reviewed Jun 14, 2021

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

bjorn3 reviewed Jun 14, 2021

View reviewed changes

CohenArthur and others added 8 commits June 14, 2021 11:37

ci: Add basic coding style CI

bfc74e3

git: Add tests and ignore their targets

70fa255

gccrs: Add argument abstraction

0920ef7

gccrs: Add actual argument parser on getopts and correct gccrs spawning

292f4d5

args: Remove "lib" as an alias for "dylib"

0344286

Co-authored-by: bjorn3 <bjorn3@users.noreply.github.com>

clippy: Apply suggestions

00adfaf

Co-authored-by: flip1995 <hello@philkrones.com>

error: Add Error type and error handling

b48c11f

Co-authored-by: flip1995 <hello@philkrones.com>

args: Remove format_output_filename() from GccrsArgs's implementation

835dcb7

Co-authored-by: flip1995 <hello@philkrones.com>

CohenArthur force-pushed the compile-basic-hw branch from d41b977 to 835dcb7 Compare June 14, 2021 09:45

gccrs: Add FIXME for multiple targets on fake_output

98688f7

Co-authored-by: bjorn3 <bjorn3@users.noreply.github.com>

philberty approved these changes Jun 14, 2021

View reviewed changes

CohenArthur merged commit 2cfc3db into main Jun 14, 2021

Compile basic rust project automation moved this from In progress to Done Jun 14, 2021

CohenArthur deleted the compile-basic-hw branch June 14, 2021 19:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compile basic rust project #5

Compile basic rust project #5

CohenArthur commented Jun 10, 2021 •

edited

Loading

bjorn3 Jun 10, 2021

CohenArthur Jun 10, 2021

bjorn3 Jun 10, 2021 •

edited

Loading

CohenArthur Jun 10, 2021

bjorn3 Jun 10, 2021

CohenArthur Jun 11, 2021

flip1995 left a comment

philberty commented Jun 11, 2021 •

edited

Loading

CohenArthur commented Jun 11, 2021

flip1995 left a comment

flip1995 left a comment

flip1995 commented Jun 14, 2021

bjorn3 Jun 14, 2021

CohenArthur Jun 14, 2021

Compile basic rust project #5

Compile basic rust project #5

Conversation

CohenArthur commented Jun 10, 2021 • edited Loading

bjorn3 Jun 10, 2021

Choose a reason for hiding this comment

CohenArthur Jun 10, 2021

Choose a reason for hiding this comment

bjorn3 Jun 10, 2021 • edited Loading

Choose a reason for hiding this comment

CohenArthur Jun 10, 2021

Choose a reason for hiding this comment

bjorn3 Jun 10, 2021

Choose a reason for hiding this comment

CohenArthur Jun 11, 2021

Choose a reason for hiding this comment

flip1995 left a comment

Choose a reason for hiding this comment

philberty commented Jun 11, 2021 • edited Loading

CohenArthur commented Jun 11, 2021

flip1995 left a comment

Choose a reason for hiding this comment

flip1995 left a comment

Choose a reason for hiding this comment

flip1995 commented Jun 14, 2021

bjorn3 Jun 14, 2021

Choose a reason for hiding this comment

CohenArthur Jun 14, 2021

Choose a reason for hiding this comment

CohenArthur commented Jun 10, 2021 •

edited

Loading

bjorn3 Jun 10, 2021 •

edited

Loading

philberty commented Jun 11, 2021 •

edited

Loading