Skip to content

Conversation

@celinval
Copy link
Contributor

@celinval celinval commented Feb 3, 2023

Description of changes:

We should probably convert this into a table format, but I think having the information is worthwhile. For this PR, the output of scan for the scan regression script looks like:

Running assess scan test:
... (redacted)
Assessed 2 successfully, with 2 failures.
Failed to assess packages:
  - `compile_error`: Failed to execute cargo (exit status: 101). Found 3 compilation errors.
  - `manifest_error`: Failed to get cargo metadata.

============================================
 Unsupported feature |   Crates | Instances
                     | impacted |    of use
---------------------+----------+-----------
 caller_location     |        2 |         2
 try                 |        2 |         2
============================================
... (redacted)

Resolved issues:

Fixes #2165

Related RFC:

Optional #ISSUE-NUMBER.

Call-outs:

There wasn't any change to the output when there is no failure.

Testing:

  • How is this change tested?

  • Is this a refactor change?

Checklist

  • Each commit message has a non-empty body, explaining why the change was made
  • Methods or procedures are documented
  • Regression or unit tests are included, or existing tests cover the modified code
  • My PR is restricted to a single feature or bugfix

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.

I still need to convert this into a table format, but I think having the
information is worthwhile.
@celinval celinval requested a review from a team as a code owner February 3, 2023 02:08
Copy link
Contributor

@tedinski tedinski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable. Notes:

  1. We're adding more to the scan test script, but should maybe be adopting Adrian's testing mode. Not for this PR, though.
  2. Besides a table, I think we should print the path to the log file for that crate that scan produces, so more information can be easily found beyond just what's printed.
  3. Have you tried this on "real" failures? What does it look like (e.g. for top-100? Which you can run with that script)

I'd consider adding the log file to the output, but beyond that lgtm

@celinval
Copy link
Contributor Author

celinval commented Feb 3, 2023

Is it Ok if I add the path if we run with verbose on. What do you think? For the top 100 crates I'm worried this is going to get way too verbose.

@celinval
Copy link
Contributor Author

celinval commented Feb 4, 2023

FYI, this is how the top 100 looks like:

Assessed 77 successfully, with 23 failures.                                                                                                                                               
Failed to assess packages:                                                                                                                                                                
  - `serde_derive`: No supported targets were found.                                                                                                                                      
  - `tokio`: Failed to execute cargo (exit status: 101). Found 5 compilation errors.                                                                                                      
  - `tokio-util`: Failed to execute cargo (exit status: 101). Found 4 compilation errors.                                                                                                 
  - `base64`: Failed to compile `base64` due to an internal compiler error.                                                                                                               
  - `hyper`: Failed to execute cargo (exit status: 101). Found 0 compilation errors.                                                                                                      
  - `unicode-bidi`: Failed to execute cargo (exit status: 101). Found 2 compilation errors.                                                                                               
  - `serde_json`: Failed to execute cargo (exit status: 101). Found 0 compilation errors.                                                                                                 
  - `pin-project-lite`: Failed to compile `compiletest` due to an internal compiler error.                                                                                                
  - `itertools`: Failed to execute cargo (exit status: 101). Found 0 compilation errors.                                                                                                  
  - `bitflags`: Failed to compile `bitflags` due to an internal compiler error.                                                                                                           
  - `futures`: Failed to execute cargo (exit status: 101). Found 0 compilation errors.                                                                                                    
  - `futures-util`: Failed to execute cargo (exit status: 101). Found 6 compilation errors.                                                                                               
  - `syn`: Failed to execute cargo (exit status: 101). Found 0 compilation errors.                                                                                                        
  - `quote`: Failed to execute cargo (exit status: 101). Found 0 compilation errors.                                                                                                      
  - `crossbeam-epoch`: Failed to compile `crossbeam-epoch` due to an internal compiler error.                                                                                             
  - `smallvec`: Failed to execute cargo (exit status: 101). Found 0 compilation errors.                                                                                                   
  - `toml`: Failed to execute cargo (exit status: 101). Found 0 compilation errors.                                                                                                       
  - `http`: Failed to compile `http` due to an internal compiler error.                                                                                                                   
  - `tracing`: Failed to execute cargo (exit status: 101). Found 0 compilation errors.                                                                                                    
  - `url`: Failed to execute cargo (exit status: 101). Found 2 compilation errors.                                                                                                        
  - `idna`: Failed to execute cargo (exit status: 101). Found 2 compilation errors.                                                                                                       
  - `thiserror-impl`: No supported targets were found.                                                                                                                                    
  - `thiserror`: Failed to compile `test_source` due to an internal compiler error.

I need to figure out what is up with the 0 compilation errors. The ones that worry me the most though are the ICE ones.

@celinval
Copy link
Contributor Author

celinval commented Feb 4, 2023

Maybe I should try to sort the output by either the crate name of the error message. @tedinski what do you think?

@celinval
Copy link
Contributor Author

celinval commented Feb 4, 2023

Ah, the 0 compilation errors seem related with project configuration issues. E.g.:

error: There are multiple `quote` packages in your project, and the specification `quote` is ambiguous.
error: no test target named `subscriber`.

I'll create an issue to track those.

Copy link
Contributor

@adpaco-aws adpaco-aws left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @celinval ! This is good information to have as well.

Also, what do you think about printing the failures after the table? IMO it would look cleaner.

@celinval
Copy link
Contributor Author

celinval commented Feb 6, 2023

Would you prefer if I print it after the table about the tests or before?

I was initially printing the error information after the unsupported constructs table, but I thought it looked a bit odd since it was in between the two tables.

@adpaco-aws
Copy link
Contributor

Oh, I didn't realize there were more tables after the first one. Then I'm OK with the PR as is.

@celinval
Copy link
Contributor Author

celinval commented Feb 6, 2023

Thanks, @celinval ! This is good information to have as well.

Also, what do you think about printing the failures after the table? IMO it would look cleaner.

That was my initial thought too, but I thought it looked a bit odd when we run the unit tests too

Oh, I didn't realize there were more tables after the first one. Then I'm OK with the PR as is.

Yeah, it depends whether we pass --only-codegen or not.

@celinval
Copy link
Contributor Author

celinval commented Feb 6, 2023

FYI, the output is now sorted by the name of the package.

@celinval celinval merged commit 68b85d6 into model-checking:main Feb 6, 2023
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.

Assess scan should include failures to build a package

3 participants