Make it available as a library?

[icefoxen]

I want to make a website that gives statistics on various crates, including unsafe usage. Naturally I immediately thought of this, but using cargo-geiger to output data and then attempting to re-parse it seems excessively hard. Would you accept a PR that refactors most of the actual work into a library crate?

I don't think I would ever really expect a stable API or such, but being able to keep all the processing in one program would be nice.

[anderejd]

Hello, nice to see you again 😄 👍

Would you accept a PR that refactors most of the actual work into a library crate?

Yes! I think this makes sense for most command line programs as well, to keep the CLI part in the main program and the rest as a library.

I don't think I would ever really expect a stable API or such, but being able to keep all the processing in one program would be nice.

Yes, let's do the 0.x dance to begin with. Keep it simple and let's do this!

[anderejd]

Oh, I just thought of a suggestion. Can we please keep both the CLI and library crate in this git repository? That would make it easier to work with and would make the code more discoverable for newcomers, in my opinion. If you would like to co-maintain I will gladly add you.

[icefoxen]

Nice to see you again as well!

And, I was intending to keep it all in one repo, I was going to just move most of main.rs into lib.rs, and add a bin/cargo-geiger.rs with all the command line handling stuff. Crate fragmentation is one of my pet peeves. ;-)

I'll gladly help co-maintain, though I'm afraid you shouldn't expect me to do that much work on stuff. I just don't have time to be too dedicated. In the interests of having a higher bus number for this project though, go ahead. I'm working on a side-project to do quality analysis of crates, so I'll probably be doing stuff in in the same general area.

[anderejd]

Excellent, collaborator invite has been sent!
My Rust time is limited at the moment so another minimal-maintainer is 100% better than me alone :)

[anderejd]

Do we need to modify the Cargo.toml to allow using only the library part from crates.io or will a new cargo publish be enough?

[icefoxen]

You know... I'm not sure. I'm pretty sure that just a new publish will be enough though.

[anderejd]

I'll prep and publish a new release.

[anderejd]

Do you have an example project using the master branch as a library?

While trying to test a minimal example program using cargo-geiger as a library, it seems to me that the cargo executable needs to launch cargo plugins as a subprocess in order to load the correct dynamic libraries. Do you have similar results?

If cargo needs to be the parent process then maybe adding some structured output format would be more useful, like optional json output perhaps?

The error I'm getting seems to be the same as this: rust-lang/rust#47931

[icefoxen]

Do you have an example project using the master branch as a library?

Honestly, I haven't tried yet. XD I've had similar issues with libraries while developing though; I need to actually install it and run with cargo geiger ... instead of being able to just do cargo run in the project dir.

[anderejd]

Honestly, I haven't tried yet.

Ok :) Well, the refactoring is a step in the right direction even if it doesn't allow running it as a normal Rust library.

I rediscovered a detail yesterday and that is that I apparently made cargo-geiger depend on not only cargo but also rustc, See https://github.com/anderejd/cargo-geiger#030 This dependency on rustc makes it a requirement to have a normal Rust build environment installed to use cargo-geiger anyways, so I'm thinking this github issue needs to be closed as won't/can't'-fix 😞

What do you think about adding optional json output as a workaround?

[anderejd]

The library rust-analyzer could become an interesting alternative to the rustc depedency since cargo-geiger doesn't need to generate any machine code. This would take cargo-geiger one step towards being able to provide a normal rust library without dependencies on pre-installed Rust build tools.

https://github.com/rust-analyzer/rust-analyzer
https://ferrous-systems.com/blog/rust-analyzer-2019/

[dpc]

Hi. I'm interested in this for cargo-crev. In the cargo crev verify deps I'd like to display a flag or something for dependencies containing unsafe to help users identify crates that need review most.

[anderejd]

Hi @dpc !

I've been following your cargo-crev project since you announced it on reddit and I'm very much looking forward to see where it's going and if it becomes part of crates.io in the long run, that could be very cool, imho. Having it part of crates.io, would be a killer feature for crates.io compared to other code/dependency package managers I have worked with.

Ok, back to business.

I'd like to display a flag or something for dependencies containing unsafe

This sounds like something that would be relatively easy to pull off. Since cargo-crev is also a cargo plugin I'm guessing that the cargo + rustc depedency shouldn't be a problem? If that is not a problem then the only thing I can think of right now that needs to happen is to separate the printing and the scanning in cargo-geiger. Would you like to attempt a PR for that or would you prefer that I do it?

[dpc]

Would you like to attempt a PR for that or would you prefer that I do it?

I am always tight on time. If you can do it, I'll take it. If you can't, I'll try to get to it at some point. :)

[anderejd]

Then I'll try to put in some time for this during the coming week or so. That refactoring has been bugging me for quite a while, guess it's time to just do it 😆

[anderejd]

The #41 was just merged and I think that should make cargo-geiger almost usable as a library now, almost. The public API is in very rough shape and I would like some help to polish that.

Do you, @icefoxen and @dpc, think the master branch is in good enough shape to start experimenting with using it as a library now?

[dpc]

Good enough to me. :)

[anderejd]

Cool :) I'm looking forward to see what you make of it.

[dpc]

Any chance for one function, that can scan one path? I need to get gieger count per standalone crate, that I already have pin-pointed. fn find_unsafe seems like the closest thing, but it's private and I'm not sure if some of these arguments are internal or not.

[icefoxen]

I second @dpc 's request there; it's nice to be able to deal with a single package at a time without all its dependencies. Apart from that, looks very nice.

[anderejd]

Sure sounds very reasonable. The only reason I made it private was to start removing things from the public API since the current version unintentionally exposes quite a few implementation details.

I will have more time for this tonight but feel free to make PRs for API changes ;)

fn find_unsafe seems like the closest thing

Yepp, that's it. I think It needs to be chopped into smaller more focused functions and move the HashSet insertion to the caller. The function should return an impl Iterator<Item = Something appropriate> too let the caller build the HashSet. And it should be made public.

Ok, that was a undercaffeinated comment on my part. First things first. It should be made public and maybe it should be refactored into something nicer.

https://github.com/anderejd/cargo-geiger/blob/master/src/lib.rs#L301

[anderejd]

Okay, ☕️ has been administered.

This comment is a note for myself as much as part of the conversation so sorry for the spam.

fn find_unsafe should not care about if a file is used in the build or not. That is a separate concern from scanning and should be handled entirely by the caller. fn find_unsafe should return something like impl Iterator<Item = (source file path, unsafe stats)>.

[anderejd]

Any chance for one function, that can scan one path? I need to get gieger count per standalone crate, that I already have pin-pointed

Now after #45 I think the cargo-crev use-case could be covered by the public API.

1. Call find_rs_files_in_packagefind_rs_files_in_dir
2. Use the iterator from step 1 to send file names to find_unsafe_in_file
3. Summarize the stats from step 2 in the caller code

Thoughts?

[dpc]

find_unsafe_in_dir looks like what I want. :)

While I was looking at it... why so many unwraps and expects? :P

[anderejd]

find_unsafe_in_dir looks like what I want. :)

...I just refactored that into other functions... I can add it back if you'd like? 😆 Were you looking at the latest commit in master?

While I was looking at it... why so many unwraps and expects? :P

Lack of time 🤷‍♂️ But yes, they should be replaced by Result.

EDIT: I just edited my previous comment.

[dpc]

https://github.com/dpc/crev/tree/geiger

Works OK for a lot of dependencies of crev itself, but unfortunately it panics somewhere.

thread 'main' panicked at 'Failed to parse file: /home/dpc/.cargo/registry/src/github.com-1ecc6299db9ec823/mio-0.6.16/test/benchmark.rs, Error { start_span: Span, end_span: Span, message: "LexEr
ror" } ', /home/dpc/.cargo/git/checkouts/cargo-geiger-d5fc9052318594d4/c891bdd/src/lib.rs:346:13
stack backtrace:                                     
   0: std::sys::unix::backtrace::tracing::imp::unwind_backtrace
             at src/libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1: std::sys_common::backtrace::_print  
             at src/libstd/sys_common/backtrace.rs:71
   2: std::panicking::default_hook::{{closure}}
             at src/libstd/sys_common/backtrace.rs:59
             at src/libstd/panicking.rs:211                                        
   3: std::panicking::default_hook
             at src/libstd/panicking.rs:227
   4: std::panicking::rust_panic_with_hook 
             at src/libstd/panicking.rs:495
   5: std::panicking::continue_panic_fmt      
             at src/libstd/panicking.rs:398
   6: std::panicking::begin_panic_fmt      
             at src/libstd/panicking.rs:353
   7: cargo_geiger::find_unsafe_in_file
             at /home/dpc/.cargo/git/checkouts/cargo-geiger-d5fc9052318594d4/c891bdd/src/lib.rs:346
   8: cargo_crev::get_geiger_count::{{closure}}                                    
             at cargo-crev/src/main.rs:583
   9: <resiter::flat_map::FlatMapOk<I, U, F> as core::iter::iterator::Iterator>::next
             at /home/dpc/.cargo/registry/src/github.com-1ecc6299db9ec823/resiter-0.3.0/src/flat_map.rs:75
  10: cargo_crev::get_geiger_count           
             at cargo-crev/src/main.rs:582
  11: cargo_crev::run_command::{{closure}}                  
             at cargo-crev/src/main.rs:755        
  12: cargo_crev::Repo::for_every_non_local_dep_crate

[anderejd]

Thanks for the report!

It looks like the time has come to remove the last unwraps and excepts. I can probably find the time to look at it this weekend. Don't be shy about making a PR for this in the meantime :)

[anderejd]

I'll be removing panics and doing some more public API polish here: #48 this weekend.

[anderejd]

@dpc Please take a peek at #49

[anderejd]

Merged #48. find_unsafe_in_file shouldn't panic now. I haven't looked into why syn return an error yet.

[dpc]

I'll try it out at some point. :) Thanks!

[anderejd]

The library parts that have been decoupled from the cargo plugin are now released on crates.io: https://crates.io/crates/geiger

I think this issue can be closed now?

[anderejd]

Closing, feel free to re-open or create new related issues! 🙂