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

Various Linux fixes #3705

Merged
merged 4 commits into from
Mar 11, 2016
Merged

Various Linux fixes #3705

merged 4 commits into from
Mar 11, 2016

Conversation

Mystro256
Copy link
Contributor

Three fixes/commits:

Thanks!

Review on Reviewable

@mposs00
Copy link
Contributor

mposs00 commented Mar 4, 2016

I don't think the version bump is necessary, 5.0 hasn't come out yet

@Mystro256
Copy link
Contributor Author

Considering it's the development branch and 5.0RC has been tagged, I figured it could be safely changed. If not, just cherry pick out that commit. The only other thing I changed in the commit was polarssl -> mbedtls, as the package was renamed in Fedora.

@Helios747
Copy link
Contributor

The RC system was scrapped.

Dolphin is a Gamecube, Wii and Triforce (the arcade machine based on the
Gamecube) emulator, which supports full HD video with several enhancements such
as compatibility with all PC controllers, turbo speed, networked multiplayer,
and more.

This comment was marked as off-topic.

@Mystro256
Copy link
Contributor Author

I've made some changes based on the input provided:

  • Reverted back to 4.0.2 in the spec
  • Used dolphin's homepage description in the manpages instead

@BhaaLseN
Copy link
Member

BhaaLseN commented Mar 4, 2016

Feel free to squash those last 3 changes into their respective commits to keep things tidy.

@jcowgill
Copy link
Contributor

jcowgill commented Mar 4, 2016

Reviewed 2 of 2 files at r3.
Review status: 2 of 4 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.


Installer/dolphin-emu-nogui.6, line 3 [r2] (raw file):
The NAME section has a particular convention you should follow. See man(7) and man-pages(7).
In effect: the executable should not be bold, and replace "is" with "-".


Installer/dolphin-emu-nogui.6, line 7 [r2] (raw file):
Usually user provided arguments are written in lowercase.


Installer/dolphin-emu-nogui.6, line 17 [r2] (raw file):
Sort the list of options?

There are also some more options not listed in the readme.
See here: https://github.com/dolphin-emu/dolphin/blob/master/Source/Core/DolphinWX/Main.cpp#L162
And here: https://sources.debian.net/data/main/d/dolphin-emu/5.0~rev8631+dfsg-1/debian/dolphin-emu.6


Installer/dolphin-emu-nogui.6, line 29 [r2] (raw file):
The commas should not be bold.


Installer/dolphin-emu-nogui.6, line 39 [r2] (raw file):
In the man page used in Debian, I've added a FILES section to briefly document the $HOME/.dolphin-emu directory just above here.


Installer/dolphin-emu.6, line 1 [r3] (raw file):
Everything I've written for dolphin-emu-nogui.6 applies here to this file as well.


Comments from the review on Reviewable.io

@delroth delroth added this to the Dolphin Release 5.0 milestone Mar 4, 2016
@Mystro256
Copy link
Contributor Author

Done, squashed into 3 commits.
Also fixed manpage issues as described above, except one issue:

  • Sort the list of options: I think just having the options in the order specified in the README and code would be the most consistent choice here. There's no de facto rule when it comes to options sorting, so I don't think it really matters, as it's still legible.

@AdmiralCurtiss
Copy link
Contributor

Google Test recommends against using precompiled system libararies, are you sure this is a good idea?

@jcowgill
Copy link
Contributor

jcowgill commented Mar 5, 2016

And then you have other distros (such as Debian / Ubuntu) which ship a shared copy of the source only in /usr/src/gtest to get around the issues Google raised. Should these distros be supported too?

@delroth
Copy link
Member

delroth commented Mar 5, 2016

In practice there is no reason to care about GTest being statically linked. It's not linked into our main binary, only in unit tests that aren't being distributed to end users.

So I would recommend reverting that part of the pull request and keeping the static linking to our Externals copy.

@Mystro256
Copy link
Contributor Author

I changed shared gtest to be an option that's disabled by default.

I can't make a solution for Debian or Ubuntu, as I don't use these distros and can't test this properly. If desired, an extra check can be added later.

@@ -952,6 +960,11 @@ if(${CMAKE_SYSTEM_NAME} STREQUAL "Linux")
DESTINATION ${CMAKE_INSTALL_PREFIX}/share/icons/hicolor/48x48/apps)
install(FILES Installer/dolphin-emu.desktop
DESTINATION ${CMAKE_INSTALL_PREFIX}/share/applications)
# Install manpages
install(FILES Installer/dolphin-emu.6
DESTINATION ${CMAKE_INSTALL_PREFIX}/usr/share/man/man6)

This comment was marked as off-topic.

This comment was marked as off-topic.

@Mystro256
Copy link
Contributor Author

Alright, I've moved the manpages to Data, and in a separate commit, I moved the Linux specific data files to Data as well.

delroth added a commit that referenced this pull request Mar 11, 2016
@delroth delroth merged commit c008d8b into dolphin-emu:master Mar 11, 2016
@adelpha
Copy link

adelpha commented Mar 12, 2016

This adds 5 files to the Windows build. Is that intended?

@Mystro256
Copy link
Contributor Author

No, I'm assuming this is an error in the windows build, where it includes everything from the Data folder (Note that I am not a windows user or developer). The windows build needs to be changed to exclude these files and not just copy the hold Data folder.

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

Successfully merging this pull request may close these issues.

8 participants