Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make it available as a library? #30

Closed
icefoxen opened this issue Dec 9, 2018 · 33 comments

Comments

Projects
None yet
3 participants
@icefoxen
Copy link
Collaborator

commented Dec 9, 2018

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

This comment has been minimized.

Copy link
Owner

commented Dec 10, 2018

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

This comment has been minimized.

Copy link
Owner

commented Dec 10, 2018

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

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 10, 2018

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

This comment has been minimized.

Copy link
Owner

commented Dec 10, 2018

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

This comment has been minimized.

Copy link
Owner

commented Dec 13, 2018

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

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 13, 2018

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

@anderejd

This comment has been minimized.

Copy link
Owner

commented Dec 13, 2018

I'll prep and publish a new release.

@anderejd

This comment has been minimized.

Copy link
Owner

commented Dec 13, 2018

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

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 13, 2018

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

This comment has been minimized.

Copy link
Owner

commented Dec 14, 2018

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

This comment has been minimized.

Copy link
Owner

commented Dec 16, 2018

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

This comment has been minimized.

Copy link

commented Jan 10, 2019

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

This comment has been minimized.

Copy link
Owner

commented Jan 10, 2019

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

This comment has been minimized.

Copy link

commented Jan 10, 2019

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

This comment has been minimized.

Copy link
Owner

commented Jan 10, 2019

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

This comment has been minimized.

Copy link
Owner

commented Jan 22, 2019

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

This comment has been minimized.

Copy link

commented Jan 22, 2019

Good enough to me. :)

@anderejd

This comment has been minimized.

Copy link
Owner

commented Jan 23, 2019

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

@dpc

This comment has been minimized.

Copy link

commented Jan 23, 2019

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

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 23, 2019

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

This comment has been minimized.

Copy link
Owner

commented Jan 23, 2019

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

This comment has been minimized.

Copy link
Owner

commented Jan 23, 2019

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

This comment has been minimized.

Copy link
Owner

commented Jan 23, 2019

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

This comment has been minimized.

Copy link

commented Jan 23, 2019

find_unsafe_in_dir looks like what I want. :)

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

@anderejd

This comment has been minimized.

Copy link
Owner

commented Jan 24, 2019

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

This comment has been minimized.

Copy link

commented Jan 25, 2019

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

This comment has been minimized.

Copy link
Owner

commented Jan 25, 2019

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

This comment has been minimized.

Copy link
Owner

commented Feb 2, 2019

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

@anderejd

This comment has been minimized.

Copy link
Owner

commented Feb 2, 2019

@dpc Please take a peek at #49

@anderejd

This comment has been minimized.

Copy link
Owner

commented Feb 2, 2019

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

@dpc

This comment has been minimized.

Copy link

commented Feb 3, 2019

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

@anderejd

This comment has been minimized.

Copy link
Owner

commented Feb 9, 2019

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

This comment has been minimized.

Copy link
Owner

commented Feb 9, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.