Merge lp://qastaging/~mterry/unity-scopes-api/snap-root into lp://qastaging/unity-scopes-api/devel

Thanks Michael! Michi and I do acknowledged this MP, but we'd like to investigate the possibility of a cleaner approach whereby we reduce some of the redundancy (especially with getenv()) that's introduced here.

This is not a stab at your work by any means ;) Thanks for your help with this!

Could you re-target this MR at the devel branch instead of trunk please? That allows us to use our normal staging process.

Is $SNAP always going to have a trailing slash? If not, constructs such as

$SNAP@CMAKE_INSTALL_PREFIX@/@CMAKE_INSTALL_LIBDIR@/@UNITY_SCOPES_LIB@/scoperegistry

and

snap_root + base_dflt_file

won't create the path name correctly. Wouldn't it be safer to append a slash regardless? If we end up with two adjacent slashes, they won't do any harm.

This does not work:

const string snap_root = getenv("SNAP");

If SNAP isn't set, getenv() returns a nullptr, which causes the string constructor to throw. That causes a bunch of tests to fail.

The repeated calls to getenv("SNAP") are a bit of a pain (and dangerous because of the needed check for a nullptr return). It would be better to do this is ConfigBase and and provide a protected accessor for the value. That puts the code to sanitize the value in a single place.

Here is a diff against this branch that makes some changes. Could you review please?

Thanks,

Michi.

> Is $SNAP always going to have a trailing slash? If not, constructs such as
> [snip[
> won't create the path name correctly. Wouldn't it be safer to append a slash
> regardless? If we end up with two adjacent slashes, they won't do any harm.

@CMAKE_INSTALL_PREFIX@ is guaranteed to either be empty or start with a slash, right? So that's fine. And base_dflt_file always starts with a slash. So I think we're fine there.

> If SNAP isn't set, getenv() returns a nullptr, which causes the string
> constructor to throw. That causes a bunch of tests to fail.

Ick, right you are. I wrote so many of these in a row, I got confused between QString and std::string.

> It would be better to do this is ConfigBase and and provide a protected
> accessor for the value. That puts the code to sanitize the value in a single
> place.

OK.

> Here is a diff against this branch that makes some changes. Could you review please?

I would go make your suggested changes, but it sounds like you were trying to upload a diff to make them for me? I don't see one. I don't think LP MPs can have attachments.

So sorry, got distracted by my wife with an imap issue in the middle of things.

It's a it of a let-down that I can't add an attachment to comments here.

Here's a link to the diff:

https://www.dropbox.com/s/qbbs3yeo6f5ovom/snap_root.diff?dl=0

> @CMAKE_INSTALL_PREFIX@ is guaranteed to either be empty or start with a slash,
> right? So that's fine. And base_dflt_file always starts with a slash. So I
> think we're fine there.

Not necessarily. I can set it to anything when I run the configure step.

I think having the slash there is safer, and it won't do any harm if there is an extra one.

Thanks! I've applied your patch.

Still need to re-target this at the devel branch, otherwise Jenkins won't test it. Could you do that please?