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

Fix get_exe_dir on OS X #440

Merged
merged 1 commit into from Aug 31, 2015

Conversation

Projects
None yet
2 participants
@pquentin
Contributor

pquentin commented Aug 3, 2015

This allows ./wesnoth to work out of the box on OS X: it will use the git checkout as the data directory.

@pquentin

This comment has been minimized.

Show comment
Hide comment
@pquentin

pquentin Aug 3, 2015

Contributor

Two builders failed due to apt-get timeouts. @aquileia was nice enough to ask another build. This time, yet another builder failed. I think it's safe to consider that the tests passed. The function I modified does not seem to be tested anyway. Should I add tests?

Contributor

pquentin commented Aug 3, 2015

Two builders failed due to apt-get timeouts. @aquileia was nice enough to ask another build. This time, yet another builder failed. I think it's safe to consider that the tests passed. The function I modified does not seem to be tested anyway. Should I add tests?

@shikadiqueen

This comment has been minimized.

Show comment
Hide comment
@shikadiqueen

shikadiqueen Aug 3, 2015

Member

So I take it that there is no procfs in Apple OS X so /proc/self/exe doesn't exist?

Also, would it not be more appropriate to fallback to get_cwd() for any situation where /proc/self/exe doesn't exist/isn't a symlink regardless of platform?

Member

shikadiqueen commented Aug 3, 2015

So I take it that there is no procfs in Apple OS X so /proc/self/exe doesn't exist?

Also, would it not be more appropriate to fallback to get_cwd() for any situation where /proc/self/exe doesn't exist/isn't a symlink regardless of platform?

@pquentin

This comment has been minimized.

Show comment
Hide comment
@pquentin

pquentin Aug 3, 2015

Contributor

@shikadilord thanks for the review!

No procfs on OS X. I changed the patch to detect the feature instead of the whole OS, that was a good idea. :) It works on OS X, I guess Travis will test it on Linux.

Contributor

pquentin commented Aug 3, 2015

@shikadilord thanks for the review!

No procfs on OS X. I changed the patch to detect the feature instead of the whole OS, that was a good idea. :) It works on OS X, I guess Travis will test it on Linux.

@shikadiqueen

This comment has been minimized.

Show comment
Hide comment
@shikadiqueen

shikadiqueen Aug 7, 2015

Member

Sorry, I forgot to come back to this.

A problem I see with this on Windows in particular right now is that in the event that the current drive (usually but not always the OS install volume, e.g. C:) has a \procdirectory, get_exe_dir() will return an empty string instead of the result of get_cwd(), which may break things. So, if you have the chance, could you restore the previous Windows behavior? Otherwise I consder this ready for merging.

(get_exe_dir() really should go the extra mile on Windows and call GetModuleFileName() to obtain this information instead of assuming the working dir has the exe, but this requires us to deal with Unicode conversion and stuff. I'll probably try this out later.)

Member

shikadiqueen commented Aug 7, 2015

Sorry, I forgot to come back to this.

A problem I see with this on Windows in particular right now is that in the event that the current drive (usually but not always the OS install volume, e.g. C:) has a \procdirectory, get_exe_dir() will return an empty string instead of the result of get_cwd(), which may break things. So, if you have the chance, could you restore the previous Windows behavior? Otherwise I consder this ready for merging.

(get_exe_dir() really should go the extra mile on Windows and call GetModuleFileName() to obtain this information instead of assuming the working dir has the exe, but this requires us to deal with Unicode conversion and stuff. I'll probably try this out later.)

@shikadiqueen shikadiqueen self-assigned this Aug 7, 2015

@pquentin

This comment has been minimized.

Show comment
Hide comment
@pquentin

pquentin Aug 10, 2015

Contributor

@shikadilord What about checking for /proc/self/exe?

Also, to be sure I understood you, is this what you're proposing here?

#ifdef _WIN32 
    return get_cwd()
#else
    if (bfs::exists("/proc/")) {
        // procfs-specific code
    } else {
        return get_cwd();
    }
#endif
Contributor

pquentin commented Aug 10, 2015

@shikadilord What about checking for /proc/self/exe?

Also, to be sure I understood you, is this what you're proposing here?

#ifdef _WIN32 
    return get_cwd()
#else
    if (bfs::exists("/proc/")) {
        // procfs-specific code
    } else {
        return get_cwd();
    }
#endif
@shikadiqueen

This comment has been minimized.

Show comment
Hide comment
@shikadiqueen

shikadiqueen Aug 12, 2015

Member

I checked the code again .If get_exe_dir() returns an empty string, this signals main() in wesnoth.cpp to not try to guess the data dir path from the executable/cwd path and go with a compiled-in default such as /usr/local/share/wesnoth instead. The compiled-in default on Windows is by default an empty string, so any paths formed from that are relative to the cwd anyway, but we should play on the safe side and make get_exe_dir return something on Windows anyway, especially since the platform does not support anything even remotely resembling a procfs (except perhaps when using Cygwin, but we don't support that).

So, yes to your second question. As for the first, it should keep returning the empty string, even if in the end this feels like a leading 0 situation.

Member

shikadiqueen commented Aug 12, 2015

I checked the code again .If get_exe_dir() returns an empty string, this signals main() in wesnoth.cpp to not try to guess the data dir path from the executable/cwd path and go with a compiled-in default such as /usr/local/share/wesnoth instead. The compiled-in default on Windows is by default an empty string, so any paths formed from that are relative to the cwd anyway, but we should play on the safe side and make get_exe_dir return something on Windows anyway, especially since the platform does not support anything even remotely resembling a procfs (except perhaps when using Cygwin, but we don't support that).

So, yes to your second question. As for the first, it should keep returning the empty string, even if in the end this feels like a leading 0 situation.

@pquentin

This comment has been minimized.

Show comment
Hide comment
@pquentin

pquentin Aug 30, 2015

Contributor

@shikadilord Updated. (What does backport mean?)

Contributor

pquentin commented Aug 30, 2015

@shikadilord Updated. (What does backport mean?)

Fix get_exe_dir on OS X
Except on Windows, this detects procfs before using it instead of trying
to detect the OS.
@shikadiqueen

This comment has been minimized.

Show comment
Hide comment
@shikadiqueen

shikadiqueen Aug 31, 2015

Member

It means it's a candidate for backporting to 1.12. That's really more a note for myself than something you need to worry about.

Member

shikadiqueen commented Aug 31, 2015

It means it's a candidate for backporting to 1.12. That's really more a note for myself than something you need to worry about.

shikadiqueen added a commit that referenced this pull request Aug 31, 2015

@shikadiqueen shikadiqueen merged commit 385cd42 into wesnoth:master Aug 31, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pquentin

This comment has been minimized.

Show comment
Hide comment
@pquentin

pquentin Aug 31, 2015

Contributor

@shikadilord Thank you for the merge and thank you for your attention to detail.

Contributor

pquentin commented Aug 31, 2015

@shikadilord Thank you for the merge and thank you for your attention to detail.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment