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

Allow source files to be found system-wide and don't hardcode /usr prefix on UNIX #93

Merged
merged 2 commits into from
Dec 16, 2024

Conversation

chewi
Copy link
Contributor

@chewi chewi commented Dec 15, 2024

/usr/share/{NCINE_LINUX_PACKAGE}/Source is tried before ~/.local/share/{NCINE_LINUX_PACKAGE}/Source, which is slightly counter-intuitive but necessary. If you favour the user directory, it will be automatically created on first launch and the system-wide directory will never be considered again, even if the user directory is still empty. The game shouldn't try to create the system-wide directory because it wouldn't have permission to do so.

@deathkiller deathkiller self-assigned this Dec 15, 2024
@deathkiller
Copy link
Owner

Thanks for contribution!

  • It seems AppImage build is broken because of changes in InitializePaths().
    It looks like you simplified the NCINE_PACKAGED_CONTENT_PATH section too much. I'd rather leave it the way it was, because it's special flag for AppImages and splitting the section into 2 #ifs makes the code less readable for me.
  • I'm a bit afraid to merge these changes, because some parts are shared across all supported platforms and Linux distributions (and files in Shared directory are shared across all my projects).
  • Do you know what values CMAKE_INSTALL_PREFIX can have on different systems? Is it set to "/usr/" everywhere by default? Is CMAKE_INSTALL_PREFIX needed also in "/lib/os-release"?
  • I'm not sure about prioritizing the system-wide Source directory over the user's, because then you would have to uninstall the system-wide Source first to be able to provide your own (e.g. if you want to test different versions). I guess I'll have to think about it a little more.

This is tried before ~/.local/share/{NCINE_LINUX_PACKAGE}/Source, which
is slightly counter-intuitive but necessary. If you favour the user
directory, it will be automatically created on first launch and the
system-wide directory will never be considered again, even if the user
directory is still empty. The game shouldn't try to create the
system-wide directory because it wouldn't have permission to do so.
@chewi chewi force-pushed the source-path branch 2 times, most recently from 39386ff to 53a55de Compare December 15, 2024 21:46
@chewi
Copy link
Contributor Author

chewi commented Dec 15, 2024

It seems AppImage build is broken because of changes in InitializePaths(). It looks like you simplified the NCINE_PACKAGED_CONTENT_PATH section too much. I'd rather leave it the way it was, because it's special flag for AppImages and splitting the section into 2 #ifs makes the code less readable for me.

Okay, I dropped that commit. It wasn't essential.

I'm a bit afraid to merge these changes, because some parts are shared across all supported platforms and Linux distributions (and files in Shared directory are shared across all my projects).

I'm not sure what you mean. Apart from the target_compile_definitions line, these changes are all under DEATH_TARGET_UNIX. I think they make sense for any Unix-like platform.

Do you know what values CMAKE_INSTALL_PREFIX can have on different systems? Is it set to "/usr/" everywhere by default? Is CMAKE_INSTALL_PREFIX needed also in "/lib/os-release"?

Actually, the default set by CMake is /usr/local, which means that as things stand right now, the game looks in the wrong place for Content unless you explicitly pass -DCMAKE_INSTALL_PREFIX=/usr to CMake. Distribution packages will do this anyway, but users building it manually probably won't. This change is therefore a good thing. I made this change to support Gentoo Prefix, which installs packages to arbitrary locations like /foo/bar/usr.

Good point about os-release though. Using CMAKE_INSTALL_PREFIX here would make sense for Gentoo Prefix, but probably not for anything else. I've reverted that bit.

I'm not sure about prioritizing the system-wide Source directory over the user's, because then you would have to uninstall the system-wide Source first to be able to provide your own (e.g. if you want to test different versions). I guess I'll have to think about it a little more.

Yeah, I didn't really want to do it that way, but I couldn't think of an alternative without dropping the automatic directory creation. Even then, users who have already run the game once without any files won't see any system-wide files installed subsequently. It would need to check whether the directory was empty or not. You don't currently have a helper for checking directory contents. If I was to start writing a new game, I'd probably use something like PhysicsFS, but I wasn't going to go that far here. 😄

@deathkiller
Copy link
Owner

I think I can change it to check presence of "Anims.j2a" or "AnimsSw.j2a" in the directory in InitializePaths(), something like this:

jazz2-native/Sources/Main.cpp

Lines 1009 to 1012 in bdf0ebd

String animsPath = fs::FindPathCaseInsensitive(fs::CombinePath(resolver.GetSourcePath(), "Anims.j2a"_s));
if (!fs::IsReadableFile(animsPath)) {
animsPath = fs::FindPathCaseInsensitive(fs::CombinePath(resolver.GetSourcePath(), "AnimsSw.j2a"_s));
}

Then the priority would be as expected (user first).
Downside is that it would have to iterate through all files in the directory twice, because Linux is usually case sensitive. I'm not sure how much it would slow down the startup, hopefully not much.

I'm not sure what you mean. Apart from the target_compile_definitions line, these changes are all under DEATH_TARGET_UNIX. I think they make sense for any Unix-like platform.

I meant mainly FreeBSD which I know is not very popular, but the game works there.

Actually, the default set by CMake is /usr/local, which means that as things stand right now, the game looks in the wrong place for Content unless you explicitly pass -DCMAKE_INSTALL_PREFIX=/usr to CMake. Distribution packages will do this anyway...

If you think nobody will complain, then it's fine for me.

I looked at PhysicsFS, but I didn't like it, I don't remember why and I wanted migrate C# code as fast as possible, so I created my own IO API. The advantage is that I can extend it as much as I want.

@deathkiller
Copy link
Owner

I'll merge this PR and adjust it in the next commits.

@deathkiller deathkiller merged commit ce3c6ac into deathkiller:master Dec 16, 2024
48 of 52 checks passed
@chewi chewi deleted the source-path branch December 16, 2024 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants