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

Add function to set path from which the config file is read #1051

Merged
merged 1 commit into from Oct 22, 2019

Conversation

@lionel1704
Copy link
Member

lionel1704 commented Oct 21, 2019

No description provided.

@lionel1704 lionel1704 requested a review from nbaksalyar as a code owner Oct 21, 2019
@lionel1704 lionel1704 closed this Oct 21, 2019
@lionel1704 lionel1704 force-pushed the lionel1704:mobile-cfg branch from 6a4c46a to 93ed053 Oct 21, 2019
@lionel1704 lionel1704 reopened this Oct 21, 2019
Copy link
Contributor

m-cat left a comment

Looks good, some minor comments.

safe_core/src/config_handler.rs Outdated Show resolved Hide resolved
safe_core/src/config_handler.rs Outdated Show resolved Hide resolved
safe_core/src/config_handler.rs Outdated Show resolved Hide resolved
safe_core/src/config_handler.rs Show resolved Hide resolved
lazy_static! {
static ref CONFIG_DIR_PATH: Mutex<Option<PathBuf>> = Mutex::new(None);
static ref SAFE_CORE_PROJECT_DIRS: Option<ProjectDirs> =
if let Some(custom_path) = unwrap!(CONFIG_DIR_PATH.lock()).clone() {

This comment has been minimized.

Copy link
@m-cat

m-cat Oct 22, 2019

Contributor

This doesn't seem right to me -- we only calculate SAFE_CORE_PROJECT_DIRS once, after which the value of CONFIG_DIR_PATH can change. I think the part that depends on CONFIG_DIR_PATH should go back to the function, the only thing I had in mind for putting in lazy_static was

ProjectDirs::from(
    CONFIG_DIR_QUALIFIER,
    CONFIG_DIR_ORGANISATION,
    CONFIG_DIR_APPLICATION,
)

as in safe_vault.

This comment has been minimized.

Copy link
@lionel1704

lionel1704 Oct 22, 2019

Author Member

Ah I see what you mean. My bad.
But we'll still need two. One for safe_core and one for safe_vault. They will be the fallback values if CONFIG_DIR_PATH is not set, yes?

This comment has been minimized.

Copy link
@m-cat

m-cat Oct 22, 2019

Contributor

Yep, exactly.

@m-cat
m-cat approved these changes Oct 22, 2019
Copy link
Contributor

m-cat left a comment

Oops, approved too quickly. Still waiting on that one change 😛

platforms

- add set_config_dir_path API to set the directory from which the
safe_core.config file should be read. Note that the
set_additional_search_path API is still required since init_logging in
maidsafe_utilities still uses config_file_handler.
- update to quic_p2p 0.3.0
@lionel1704 lionel1704 force-pushed the lionel1704:mobile-cfg branch from 97de3e6 to 95ec8b5 Oct 22, 2019
@lionel1704 lionel1704 requested a review from m-cat Oct 22, 2019
@m-cat
m-cat approved these changes Oct 22, 2019
/// Sets the path from which the `safe_core.config` file will be read.
#[no_mangle]
pub unsafe extern "C" fn auth_set_config_dir_path(
new_path: *const c_char,
user_data: *mut c_void,
o_cb: extern "C" fn(user_data: *mut c_void, result: *const FfiResult),
) {
catch_unwind_cb(user_data, o_cb, || -> Result<_, AuthError> {
let new_path = CStr::from_ptr(new_path).to_str()?;
config_handler::set_config_dir_path(OsStr::new(new_path));
o_cb(user_data, FFI_RESULT_OK);
Ok(())
});
}

Comment on lines +155 to +169

This comment has been minimized.

Copy link
@loziniak

loziniak Oct 22, 2019

Seems like violation of DRY principle.

This comment has been minimized.

Copy link
@lionel1704

lionel1704 Oct 22, 2019

Author Member

Hi @loziniak
Thanks for your review. The repetition is because this is a step towards deprecating auth_set_additional_search_path.

As of this PR the reading of the config file is migrated but maidsafe_utilities::init_logging still uses the path from set_additional_search_path. That will be deprecated in the near future.

This comment has been minimized.

Copy link
@m-cat

m-cat Oct 22, 2019

Contributor

@loziniak Do you mean the duplication of this function between safe_auth and safe_app?

This comment has been minimized.

Copy link
@loziniak

loziniak Oct 22, 2019

@m-cat yes, they look exactly the same. But now, when I think about it, there is mostly bilerplate code, so duplication is probably not so important here...

This comment has been minimized.

Copy link
@m-cat

m-cat Oct 22, 2019

Contributor

Yeah, we have some ideas on reducing the FFI boilerplate but we may not get around to it for a while. As for the two separate functions, I think the repetition here is unfortunately necessary as safe_auth and safe_app are two separate libraries. Thanks for pointing this out!

@lionel1704 lionel1704 merged commit e97561c into maidsafe:master Oct 22, 2019
2 checks passed
2 checks passed
Travis CI - Pull Request Build Passed
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
@lionel1704 lionel1704 deleted the lionel1704:mobile-cfg branch Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.