do not migrate legacy homepath when using -homepath arg

[illwieckz]

The bug is obvious:

In #20 (see also #39) I modified DefaultHomePath() in src/common/FileSystem.cpp to move legacy homepath to XDG's .local/share/unvanquished path (or to follow XDG env vars). But DefaultHomePath() is called in cmdlineArgs_t initialization in src/engine/framework/System.cpp and cmdlineArgs is initialized in ParseCmdline() in src/engine/framework/System.cpp before -homepath switch is parsed. So, the engine try to do the default homepath move even if an arbitrary homepath is forced by command line. Which is wrong.

This fix is an attempt to fix that this way:

• DefaultHomePath() only returns the xdgpath, so cmdlineArgs can be initialized without doing the legacy homepath move;
• an MigrateHomePath() function was created to do the homepath move (if engine built for linux) to be called by FS::Initialize();
• MigrateHomePath() only do the move if cmdLineArgs.homePath equals defaultHomePath (it means another path was not enforced) so it means the move will not be done if a custom homepath is set.

I'm currently unable to build the engine, so if someone can build and test it would be very cool. By the way it does not prevent to have a first look at the code to tell if something is obviously wrong. 😛

[slipher]

I think it should actually do what the description says, which is to not migrate if the -homepath arg is specified (rather than not migrating if some string compares equal).

[slipher]

Also, you can see the compiler errors by following the Travis links.

[illwieckz]

Hmm, or perhaps we have to rewrite the description according to the behavior?

Currently the code tries to find a legacy homepath to migrate it if the current homepath is the default one, even if the current homepath is the default one because of an useless -homepath option that is just reproducing the default behavior (the same way people can pass useless -net 29760 arg just to keep it explicit in scripts even if it's useless).

The question is: does it make sense to not migrate the legacy homepath (${HOME}/.unvanquished) when someone is uselessly setting the default homepath (${XDG_DATA_HOME:-${HOME}/.local/share}/unvanquished) by command line (doing ./daemon -homepath "${XDG_DATA_HOME:-${HOME}/.local/share}/unvanquished")?

If it does not makes sense to not migrate the legacy homepath if Daemon is using the xdg one (because being set by default or explicitly set, this behavior is acceptable and the description has to be updated.

Otherwise if we want to disable the homepath migration as soon as there is one explicit -homepath arg, the code has to be rewritten yes. But is there a reason for that?

[slipher]

If you use the -homepath flag, should that not set the homepath, instead of being randomly ignored sometimes?

[illwieckz]

I'm not sure to have understood the question… 😮

-homepath always sets the homepath and I never seen it ignored… 😳

[slipher]

Sorry, my previous comment didn't quite make sense. But it results in inconsistent behavior to use a string comparison on the filename, since you can refer to the same path by multiple spellings.

It's not that big of a deal if a hacky string comparison is used, assuming this is temporary code we're going to delete after the next release. On the other hand, it's not much effort to check whether the argument was actually set, considering that -homepath is already checked in its own if-statement in ParseCmdline().

[illwieckz]

Oh yes now I understand what your are talking about, and it also brings to my mind the issue we can have with symlinks (and @IngarKCT is known to symlink .local/share ( ͡° ͜ʖ ͡°)).

So, it probably far better to check for the presence of that switch than relying on hacky string comparison.

It's not that big of a deal if a hacky string comparison is used, assuming this is temporary code we're going to delete after the next release.

Yeah, that's also why I prefer having this code in a dedicated function we can drop easily (unlike the first attempt).

[illwieckz]

Updated the PR with a “if -homepath not in cmdline args then migrate” behavior.
Travis looks happy now, but I'm still unable to build and test the code myself…

[slipher]

You can sidestep the build issue by turning off precompiled headers.

[illwieckz]

I'm now able to build and test and the code works as expected on my end.

[mjstahlberg]

As with Unvanquished/updater#13, I'm generally not happy with including one-time-use migration code if it can be avoided. In this case it seems that it cannot, but you might want to create an unvanquished branch for the upcoming Unvanquished release only, and switch back to tagging master for the release thereafter, as the migration should affect the next Unvanquished release exclusively.

On a sidenote, both branches and tags that are unvanquished-specific should says so in their name, e.g. unv/0.51.0 for a release tag and unv/for-0.51.0 for a pre-release branch of Dæmon. This way xon and others can have their own tags and branches.