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

Modifications needed for building Kodi 18 on FreeBSD host #12992

Merged
merged 3 commits into from Nov 7, 2017
Merged

Modifications needed for building Kodi 18 on FreeBSD host #12992

merged 3 commits into from Nov 7, 2017

Conversation

ahtotaat
Copy link
Contributor

@ahtotaat ahtotaat commented Nov 1, 2017

modified:   xbmc/cores/VideoPlayer/DVDDemuxSPU.cpp
modified:   xbmc/guilib/TextureBundleXBT.h
modified:   xbmc/guilib/XBTF.h
modified:   xbmc/interfaces/builtins/WeatherBuiltins.cpp
modified:   xbmc/platform/posix/main.cpp
modified:   xbmc/system.h
modified:   xbmc/utils/CharsetConverter.cpp
modified:   xbmc/windowing/WindowingFactory.h
modified:   xbmc/windowing/X11/GLContextEGL.cpp
modified:   xbmc/windowing/X11/WinEventsX11.h
cmake/platform/freebsd/wayland.cmake
cmake/platform/freebsd/x11.cmake
cmake/scripts/freebsd/ExtraTargets.cmake
cmake/scripts/freebsd/clang-check-test.sh.in
cmake/scripts/freebsd/cppcheck-test.sh.in
xbmc/freebsd/ExtraTargets.cmake
xbmc/freebsd/x11.cmake

More-or-less minimal changes needed for getting the build finish on FreeBSD machine.

Description

Mostly added necessary macros.

Motivation and Context

Build system failed on FreeBSD right in the beginning.

How Has This Been Tested?

2 installations of FreeBSD (11.1-RELEASE and 11.1-STABLE)
1 installation of Ubuntu 16.04.3 (checked if I introduced some regression by accident)

Screenshots (if appropriate):

https://www.upload.ee/image/7616284/IMG_20171031_231900.jpg

Types of change

  • [x ] Bug fix (non-breaking change which fixes an issue)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the Code guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • [x ] I have read the CONTRIBUTING document
  • I have added tests to cover my change
  • All new and existing tests passed

@notspiff
Copy link
Contributor

notspiff commented Nov 1, 2017

Those includes are likely just implicit on gnu toolchains. If we can avoid those ifdefs..

@ahtotaat
Copy link
Contributor Author

ahtotaat commented Nov 2, 2017

I'll see how much it's possible to reduce/remove these macros.

Copy link
Member

@wsnipex wsnipex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, looks good.
As noted by notspiff, reducing the ifdefs as far as possible would be great.

if(CORE_SYSTEM_NAME STREQUAL freebsd)
set(CORE_PLATFORM_NAME X11)
endif()

if(CORE_SYSTEM_NAME STREQUAL linux)

This comment was marked as spam.

@ahtotaat ahtotaat closed this Nov 2, 2017
@Rechi
Copy link
Member

Rechi commented Nov 2, 2017

@ahtotaat you don't have to close the PR, for updating it. If you force push your changes to your branch the PR will be automatically updated.

@ahtotaat
Copy link
Contributor Author

ahtotaat commented Nov 2, 2017

Oki, thanks. I'm doing this for the first time so..

@ahtotaat ahtotaat reopened this Nov 2, 2017
@notspiff
Copy link
Contributor

notspiff commented Nov 2, 2017

no worries, about 95% of new contributors do this :)

Copy link
Member

@Rechi Rechi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please give the commits meaningful commit messages.
Also styling of the includes can be a bit improved. Separate kodi and system headers with an empty line for better readability.

#include <iconv.h>
#elif TARGET_FREEBSD
#include "/usr/include/iconv.h"
#endif

This comment was marked as spam.

This comment was marked as spam.

Copy link
Member

@wsnipex wsnipex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me

@MartijnKaijser
Copy link
Member

Small warning. Looks like @ahtotaat is using the website and is working in master branch. This may cause is issues and commits should be squashed into one or several.

@ahtotaat
Copy link
Contributor Author

ahtotaat commented Nov 4, 2017

Sigh, I may as well give up.. It's getting frustrating.

@ahtotaat ahtotaat closed this Nov 4, 2017
@Rechi
Copy link
Member

Rechi commented Nov 4, 2017

@ahtotaat any reason to close the PR?
Most of the code looks good and we can fix the commit messages for you if you can't do it yourself.

@ahtotaat
Copy link
Contributor Author

ahtotaat commented Nov 5, 2017

Reason is fairly simple. I would first have to get the "hang" of the SOP and how this system works, before submitting PR's into fairly big project.

Until then I'm pretty much running in circles. I've already had accidental commit where I only wanted to sync between home machine and laptop (I was in hospital at the time)

For example, I do not really understand what "This may cause is issues and commits should be squashed into one or several." really means yet or how to achieve that.

More trouble than worth and waste of time for everyone included.

@hudokkow
Copy link
Member

hudokkow commented Nov 5, 2017

Not a waste of time for sure.
You found something in need of fixing, you fixed it and presented the solution. Thank you for that! 👍
Yes, git/github is frustrating for newcomers but we're here to help.

If you feel up to it, please re-open the PR and we'll guide you.

@ahtotaat
Copy link
Contributor Author

ahtotaat commented Nov 6, 2017

@Rechi. If you would be willing to fix the commit messages, I'd appreciate it.

@Rechi Rechi reopened this Nov 6, 2017
@Rechi
Copy link
Member

Rechi commented Nov 6, 2017

@ahtotaat done

@Rechi Rechi added CMake Documentation Type: Improvement non-breaking change which improves existing functionality v18 Leia labels Nov 6, 2017
@Rechi Rechi added this to the L 18.0-alpha1 milestone Nov 6, 2017
@Rechi
Copy link
Member

Rechi commented Nov 7, 2017

@wsnipex still good to get merged?

@wsnipex
Copy link
Member

wsnipex commented Nov 7, 2017

yes, looks good. Thanks for the work and patience @ahtotaat

@wsnipex wsnipex merged commit 04f1647 into xbmc:master Nov 7, 2017
@ahtotaat
Copy link
Contributor Author

ahtotaat commented Nov 7, 2017

I thank you for the patience ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CMake Documentation Type: Improvement non-breaking change which improves existing functionality v18 Leia
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants