Skip to content

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

Merged
merged 2 commits into from
Mar 25, 2017

Conversation

9prady9
Copy link
Member

@9prady9 9prady9 commented Mar 21, 2017

Fixes #118

@9prady9 9prady9 requested review from pavanky and jramapuram March 21, 2017 08:34
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/")
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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?

Copy link
Member Author

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");
Copy link
Member

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 ?

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member

@jramapuram jramapuram left a 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/")
Copy link
Member

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");
Copy link
Member

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.

@9prady9 9prady9 force-pushed the build_lib_search_options branch from 99f8d03 to c9cdc36 Compare March 24, 2017 03:07
@9prady9 9prady9 requested review from pavanky and jramapuram March 24, 2017 03:08
@9prady9
Copy link
Member Author

9prady9 commented Mar 24, 2017

@pavanky @jramapuram I have made some changes based on the feedback. Please review the latest once again. Thanks.

@9prady9 9prady9 force-pushed the build_lib_search_options branch from c9cdc36 to 02cd6dd Compare March 24, 2017 03:50
@9prady9 9prady9 changed the title Add more lib dir search options for build script [WIP] Add more lib dir search options for build script Mar 24, 2017
@9prady9 9prady9 force-pushed the build_lib_search_options branch from 02cd6dd to f0b2ae2 Compare March 24, 2017 04:23
@9prady9 9prady9 requested review from pavanky and jramapuram March 24, 2017 04:24
@9prady9 9prady9 changed the title [WIP] Add more lib dir search options for build script Add more lib dir search options for build script Mar 24, 2017
@jramapuram
Copy link
Member

@9prady9 : looks good to me!

@9prady9 9prady9 merged commit d3f240d into arrayfire:devel Mar 25, 2017
@9prady9 9prady9 deleted the build_lib_search_options branch March 25, 2017 03:12
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.

3 participants