-
Notifications
You must be signed in to change notification settings - Fork 59
Add more lib dir search options for build script #119
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
Conversation
build.rs
Outdated
Err(_) => panic!("Error use_lib is defined, but AF_PATH is not defined"), | ||
Err(_) => { | ||
println!("WARNING! USE_LIB IS DEFINED, BUT AF_PATH IS NOT DEFINED, TRYING TO FIND LIBRARIES FROM KNOWN DEFAULT LOCATIONS"); | ||
PathBuf::from("/usr/") |
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 don't understand what is happening here.
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.
If there is no AF_PATH, it will look for libraries in some predefined paths for platforms. If there is AF_PATH, it will also be included into the search paths.
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.
Makes sense, but shouldn't we look into /usr/local
by default? Isn't that the default AF_PATH
?
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.
True.
build.rs
Outdated
Err(_) => { | ||
println!("WARNING! USE_LIB IS DEFINED, BUT AF_PATH IS NOT DEFINED, TRYING TO FIND LIBRARIES FROM KNOWN DEFAULT LOCATIONS"); | ||
PathBuf::from("/usr/") | ||
}, | ||
}; | ||
let libpath = afpath.join("lib"); |
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.
Will this not be an error if AF_PATH
is empty ?
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.
No, there won't be error. I think we can panic
or use some default paths. We are not checking for existence of the libraries, we are only adding paths like -L
we use for gcc/g++ link options. If required library is not present in any of search paths, it will throw linking errors.
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.
afpath
will never be None
. Under the Err it is being set to another path location. I.e. error is handled when AF_PATH
is not set.
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.
Looks good minus the one comment
build.rs
Outdated
Err(_) => panic!("Error use_lib is defined, but AF_PATH is not defined"), | ||
Err(_) => { | ||
println!("WARNING! USE_LIB IS DEFINED, BUT AF_PATH IS NOT DEFINED, TRYING TO FIND LIBRARIES FROM KNOWN DEFAULT LOCATIONS"); | ||
PathBuf::from("/usr/") |
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.
Makes sense, but shouldn't we look into /usr/local
by default? Isn't that the default AF_PATH
?
build.rs
Outdated
Err(_) => { | ||
println!("WARNING! USE_LIB IS DEFINED, BUT AF_PATH IS NOT DEFINED, TRYING TO FIND LIBRARIES FROM KNOWN DEFAULT LOCATIONS"); | ||
PathBuf::from("/usr/") | ||
}, | ||
}; | ||
let libpath = afpath.join("lib"); |
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.
afpath
will never be None
. Under the Err it is being set to another path location. I.e. error is handled when AF_PATH
is not set.
99f8d03
to
c9cdc36
Compare
@pavanky @jramapuram I have made some changes based on the feedback. Please review the latest once again. Thanks. |
c9cdc36
to
02cd6dd
Compare
02cd6dd
to
f0b2ae2
Compare
@9prady9 : looks good to me! |
Fixes #118