nba-emu / NanoBoyAdvance

A cycle-accurate Nintendo Game Boy Advance emulator.
GNU General Public License v3.0
974 stars 56 forks source link

Better Linux Desktop Integration #291

Closed JakobDev closed 11 months ago

JakobDev commented 1 year ago

This PR improves the Linux Desktop Integration. First I added the possibility build NanoBoyAdvance in a non portable mode. When doing this, the config.toml gets saved in the config directory instead of the working directory. Works also on other OS. I also added data files that are needed on the Linux Desktop. And all files can now be installed with CMake, which is common on the Linux Desktop. Note: When Installing with CMake, it also installs files from the decencies. I don't know how to block this.

fleroviux commented 1 year ago

Thanks for this PR!

When you say that the install command could install files from dependencies, does this mean that system package files i.e. for the {fmt} library could be overwritten?

@nadiaholmquist What do you think about this PR? I'm tagging you since you maintain the Arch Linux AUR package.

nadiaholmquist commented 1 year ago

This mostly seems good to me though I have a few nitpicks:

  1. Shouldn't the package name be com.github.nba_emu.NanoBoyAdvance? Leaving out that last dot looks very wrong to me
  2. You should probably add a check like (UNIX AND NOT APPLE) around the install commands, they only really make sense on systems like Linux and BSD
  3. Maybe have the portable option also turn on building the Mac bundle, as the options basically do the same thing
  4. Maybe enable portable by default on Linux/BSD
JakobDev commented 1 year ago

When you say that the install command could install files from dependencies, does this mean that system package files i.e. for the {fmt} library could be overwritten

Yes

Shouldn't the package name be com.github.nba_emu.NanoBoyAdvance? Leaving out that last dot looks very wrong to me

Yes, that's a typo. Good caught. Flathub forbidscom.github, so this is why I use io.github.

fleroviux commented 11 months ago

@JakobDev can the install command be removed for now? I don't want to ship something to users that's potentially harmful to their system. Other than that this probably could be merged (after resolving the merge conflict in config.hpp).

JakobDev commented 11 months ago

The install commands are not harmful for any system. they are only run when the user explicit executes cmake --install or make install. Otherwise they are ignored. They ensure that all data files are installed in the correct directory. Install targets are quite common in the Linux world.

fleroviux commented 11 months ago

Yes, I know that the user has to explicitly run the install command. I was referring to this:

When you say that the install command could install files from dependencies, does this mean that system package files i.e. for the {fmt} library could be overwritten

Yes

Then again I don't know if there's any real risk, but I'd rather not overwrite any existing system files by accident. Even if the user has to explicitly run the command, they might not expect libraries that already are installed in the system to be overwritten.

JakobDev commented 11 months ago

I've now found a way to not install things from the subprojects, so only the 4 files from NBA will be installed

fleroviux commented 11 months ago

Thanks! LGTM