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

Enable relocatable linux builds #3247

Merged
merged 2 commits into from Nov 11, 2015

Conversation

7 participants
@strycore
Copy link
Contributor

commented Nov 10, 2015

This patch allows to build Dolphin on Linux so that it will look for the "Sys" folder where the executable is located, making it possible to run it from anywhere in the filesystem.

For this functionality to work, the LINUX_LOCAL_DEV flag has to be set, otherwise it will keep the default behavior.

@@ -147,6 +147,8 @@ std::string GetBundleDirectory();

#ifdef _WIN32
std::string &GetExeDirectory();
#else
std::string &GetExeDirectory();
#endif

This comment has been minimized.

Copy link
@JosJuice

JosJuice Nov 10, 2015

Contributor

Instead of adding an else, you can just remove this whole ifdef.

}
Dolphin_exe_Path[len] = '\0';
DolphinPath = Dolphin_exe_Path;
DolphinPath = DolphinPath.substr(0, DolphinPath.find_last_of("/"));

This comment has been minimized.

Copy link
@JosJuice

JosJuice Nov 10, 2015

Contributor

These 7 lines aren't properly indented with tabs.

This comment has been minimized.

Copy link
@degasus

degasus Nov 10, 2015

Member

'/' instead of "/"

#else
sysDir = SYSDATA_DIR;
sysDir = SYSDATA_DIR;

This comment has been minimized.

Copy link
@JosJuice

JosJuice Nov 10, 2015

Contributor

Same with this line.

@@ -755,8 +772,10 @@ std::string GetSysDirectory()
sysDir = GetBundleDirectory() + DIR_SEP + SYSDATA_DIR;
#elif defined (_WIN32)
sysDir = GetExeDirectory() + DIR_SEP + SYSDATA_DIR;
#elif defined (LINUX_LOCAL_DEV)
sysDir = GetExeDirectory() + DIR_SEP + "Sys";

This comment has been minimized.

Copy link
@BhaaLseN

BhaaLseN Nov 10, 2015

Member

Shouldn't this be SYSDATA_DIR like on the other platforms (and so-forth be the same as the _WIN32 one)?

This comment has been minimized.

Copy link
@strycore

strycore Nov 10, 2015

Author Contributor

I tried that but the path ended up to be /path/to/dolphin//usr/local/dolphin/sys

This comment has been minimized.

Copy link
@BhaaLseN

BhaaLseN Nov 10, 2015

Member

Its a relative directory path on Windows, so it might have been redefined based on the platform defines. Maybe check there?

This comment has been minimized.

Copy link
@strycore

strycore Nov 10, 2015

Author Contributor

On linux SYSDATA_PATH is defined as #define SYSDATA_DIR DATA_DIR "sys" (in Source/Core/Common/CommonPaths.h:43) meaning that it will always be an absolute path if the DATA_DIR is defined.
Maybe the CMakeLists.txt could be modified so that DATA_DIR is unset when LINUX_LOCAL_DEV is set?

@@ -745,6 +745,23 @@ std::string& GetExeDirectory()
}
return DolphinPath;
}
#else
std::string& GetExeDirectory()

This comment has been minimized.

Copy link
@degasus

degasus Nov 10, 2015

Member

Half of the code here is shared with the windows implementation. It should be fine to only rewrite the inner part within if{}.

#else
char Dolphin_exe_Path[PATH_MAX];
ssize_t len = ::readlink("/proc/self/exe", Dolphin_exe_Path, sizeof(Dolphin_exe_Path));
if (len == -1 || len == sizeof(Dolphin_exe_Path)) {

This comment has been minimized.

Copy link
@lioncash

lioncash Nov 10, 2015

Member

We use braces on the next line (or you can elide them completely in this case)

}
Dolphin_exe_Path[len] = '\0';
DolphinPath = Dolphin_exe_Path;
DolphinPath = DolphinPath.substr(0, DolphinPath.find_last_of('/'));

This comment has been minimized.

Copy link
@lioncash

lioncash Nov 10, 2015

Member
DolphinPath = DolphinPath.substr(0, DolphinPath.rfind('/'));
@@ -755,6 +765,8 @@ std::string GetSysDirectory()
sysDir = GetBundleDirectory() + DIR_SEP + SYSDATA_DIR;
#elif defined (_WIN32)
sysDir = GetExeDirectory() + DIR_SEP + SYSDATA_DIR;
#elif defined (LINUX_LOCAL_DEV)

This comment has been minimized.

Copy link
@BhaaLseN

BhaaLseN Nov 10, 2015

Member

I'd probably do the same as in CommonPaths.h, and reuse the same block for both defines.

This comment has been minimized.

Copy link
@strycore

strycore Nov 10, 2015

Author Contributor

Right, I've pushed a fix

@BhaaLseN

This comment has been minimized.

Copy link
Member

commented Nov 10, 2015

@dolphin-emu-bot rebuild

I believe you could squash a few of those commits, but other than that I guess its ready.

Implement relocatable builds on Linux
- Change the path of the Sys folder to the executable's location
- Add LINUX_LOCAL_DEV flag to use relocatable version on Linux
- Add CMake definition for relocatable build

@strycore strycore force-pushed the lutris:master branch from cae7971 to f2ae1a2 Nov 10, 2015

# Add an option to build relocatable binaries on Linux
# The Sys folder will need to be copied to the Binaries folder.
if(UNIX)
add_definitions(-DLINUX_LOCAL_DEV=0)

This comment has been minimized.

Copy link
@jcowgill

jcowgill Nov 10, 2015

Contributor

Did you intend for this to be enabled by default? Would a CMake option be a good idea here?

This comment has been minimized.

Copy link
@strycore

strycore Nov 10, 2015

Author Contributor

Isn't it disabled by default here with the =0?
Like I said, I know very little about CMake so I'll put whatever makes the most sense.

This comment has been minimized.

Copy link
@Tilka

Tilka Nov 10, 2015

Member

Would a CMake option be a good idea here?

Yes.

This comment has been minimized.

Copy link
@strycore

strycore Nov 10, 2015

Author Contributor

So something like

option(LINUX_LOCAL_DEV "Enables relocatable build" OFF)

I suppose?

This comment has been minimized.

Copy link
@jcowgill

jcowgill Nov 10, 2015

Contributor

Isn't it disabled by default here with the =0?

No, because defined(LINUX_LOCAL_DEV) is still true even if you define it to 0.

This comment has been minimized.

Copy link
@Tilka

Tilka Nov 10, 2015

Member

So something like [...]

Yes, but you still have to add a define based on that.

This comment has been minimized.

Copy link
@strycore

strycore Nov 10, 2015

Author Contributor

Ok, I've commited some changes, hope I did it the right way.

@degasus

This comment has been minimized.

Copy link
Member

commented Nov 11, 2015

LGTM

degasus added a commit that referenced this pull request Nov 11, 2015

Merge pull request #3247 from lutris/master
Enable relocatable linux builds

@degasus degasus merged commit 8924980 into dolphin-emu:master Nov 11, 2015

11 checks passed

default Very basic checks passed, handed off to Buildbot.
Details
lint Build succeeded on the Buildbot.
Details
pr-android Build succeeded on the Buildbot.
Details
pr-deb-dbg-x64 Build succeeded on the Buildbot.
Details
pr-deb-x64 Build succeeded on the Buildbot.
Details
pr-freebsd Build succeeded on the Buildbot.
Details
pr-osx-x64 Build succeeded on the Buildbot.
Details
pr-ubu-nogui-x64 Build succeeded on the Buildbot.
Details
pr-ubu-x64 Build succeeded on the Buildbot.
Details
pr-win-dbg-x64 Build succeeded on the Buildbot.
Details
pr-win-x64 Build succeeded on the Buildbot.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.