-
Notifications
You must be signed in to change notification settings - Fork 1
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
Update for Version 0.2.0 #1
Conversation
* Added script for release builds and release profile * Added json and clap dependency * Added further package metadata * Added license and empty readme * Added format file * Added cli parsing * Added readelf stub * Added checks for required programs * Package verify is now ignoring libraries contained inside the package itself * Added data structure to seperate input, processing and output * Added json for output * Added nested parallel execution to include fetching files for packages * Listing libraries for files, files for libraries and packages for libraries * Added ability to check only selected packages * Added ability to ignore libraries
* Added enum for differnt output types * Improved Error Handling at command execution * Replaced library vector with hashset for easier dedup * Added further verbose output * Restructured json output * moved check if library is ignored or in package from cmd.rs to process.rs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow this is huge, thanks a lot!
I just quickly skipped through the code at this point, I will try this out later today. :)
build_release.sh
Outdated
@@ -0,0 +1,6 @@ | |||
#!/bin/sh | |||
cargo rustc --release |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am curious, what is the difference between cargo build --release
and cargo rustc --release
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is none :D - I was trying out cargo rustc --release -- -C prefer-dynamic
but realized that dynamic linking is not a good idea. Will change it back with the next commit
Cargo.toml
Outdated
|
||
[profile.release] | ||
lto = true | ||
panic = 'abort' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to keep codegen-units=1
It will slow down building in release mode a bit but it will speed up linking a lot and will allow for better optimizations.
rust-lang/rust#48025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw your commit for rpm - will do (it wasn't set on the arch branch yet)
Edit: done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah. The machine I made the rust prototype on runs fedora so I just used rpm to be able to do some quick "does it run" tests. Looks like I forgot to merge the arch branch, but whatever.
src/cli.rs
Outdated
.arg( | ||
Arg::with_name("group by library") | ||
.long("group_by_library") | ||
.help("groups output by librarires required in files"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: librarires
src/cli.rs
Outdated
.arg( | ||
Arg::with_name("group by containing package") | ||
.long("group_by_containing_package") | ||
.help("groups output by packages containg libraries"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: containg
src/cmd.rs
Outdated
for line in output.lines() { | ||
if line.ends_with("=> not found") { | ||
let mut library_name = String::from(line.trim()); | ||
let new_length = library_name.len() - 13; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the -13 is length of "=> not found\n"?
I would like some kind of clarifying comment where the number is coming from.
I hope in the future we can just write "hello world".len() and it resolve that to integer at compile time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind, it seems that "foo".len() works already! :D
https://godbolt.org/g/gvcivA
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay so I can instead use " => not found".len() (note the extra space so it gets to 13)
that's definitely better for clarity sake
src/cmd.rs
Outdated
dependency.library_dependencies.insert(library_name); | ||
} | ||
} | ||
if dependency.library_dependencies.len() > 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe !dependency.library_dependencies.is_empty()
is bit more idiomatic, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree
src/cmd.rs
Outdated
pub fn verify_files_via_readelf<'a>(file: &str) -> Result<Option<FileDependency>, Error<'a>> { | ||
let mut dependency = FileDependency::default(); | ||
dependency.file_name = String::from(file); | ||
if dependency.library_dependencies.len() > 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
src/main.rs
Outdated
@@ -1,5 +1,13 @@ | |||
//! A program for checking ArchLinux packages for missing libraries. | |||
//! | |||
//! If a package is missing a library it may means, that is necessary to rebuild that given package. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: it may means -> it may mean
* Changed build_release.sh from rustc to build * Added codegen-units=1 * Corrected a few linguistic mistakes * Small code readability fixes * Added non 0 exit code if a package is missing libraries
@@ -0,0 +1,21 @@ | |||
MIT License | |||
|
|||
Copyright (c) 2017 Marc Mettke |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ups that must be corrected
Hmm, is the "--all_packages" flag working? I tried |
Nevermind, I see it (missing underscore):
|
I think you can drop that. RPM was just me making sure the script works on a fedora machine, this was not intended for real world use case. :) |
I would like to keep the "just run it and it checks everything" functionality. |
Yes, no problem. Even tho I find it to be a bit counter-intuitive know. I thought about it for the bash script, 'cause it is required to be backward compatible but this is a new program with (maybe) new rules. My problem with it is, that the user probably won't expect it to run all packages. Currently, he has to specify via subcommand whether he wants to use ldd and readelf (or maybe other in the future?). There is no indication that the program will select something for him if he doesn't. But I will leave it up to you, these are my proposal: Way 1The current way, the user has to specify exactly what he wants to do or nothing happens: # Run all
aurebuildcheck_rs ldd -a
# Run for multiple packages
aurebuildcheck_rs ldd android-studio Way 2The alternative would be to always prefer something. If there is only the subcommand, it will run all packages automatically. If there is nothing it will run all packages with ldd automatically: # Resolves to "aurebuildcheck_rs ldd -a"
aurebuildcheck_rs ldd
# Resolves to "aurebuildcheck_rs ldd -a"
aurebuildcheck_rs |
But did it work as expected? |
# Conflicts: # Cargo.toml
Btw I have added an aur package for this binary. Currently, it points to my repo, but I will change that when this merge and the version tags are added. I can add you as Co-Maintainer if you want (just need your username) |
I have pushed my changed to the arch branch and created a new PR. That way you will be able to add changes as well Closing in favor of #2 |
Changes
Usage
Output
These results are rigged. Don't try to archive them yourself by running the binary, 'cause the missing libraries are included in the package itself and will be filtered out. I have removed that functionality for this output to be able to showcase something
TODO
Further changes
I will look into it a bit more when I implement readelf. But sounds reasonable to add it
That is a great idea. I will definitely take a look at regex crates for rust