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

Fix OProfile discovery and drop bfd dependency #3138

Merged
merged 1 commit into from Oct 11, 2015

Conversation

ShadowsFriend
Copy link
Contributor

The current implementation did not find the OProfile library. This adds a new cmake module to find OProfile and adjusts the CMakeLists.txt to take advantage of it.

@phire
Copy link
Member

phire commented Oct 5, 2015

Well, I guess all of the *nix buildbots are broken.

# OPROFILE_LIBRARIES - The libraries needed to use OProfile

if(CMAKE_SIZEOF_VOID_P EQUAL 8)
set(_lib_suffix 64)

This comment was marked as off-topic.

@ShadowsFriend
Copy link
Contributor Author

I honestly don't know. The check shouldn't hurt though, should it?

@phire
Copy link
Member

phire commented Oct 5, 2015

I guess not. Just seems hacky to test the size of void* There are a few ABIs that have 32bit pointers despite being 64bit, like x32 or the ps3 (not that dolphin will ever be running on either of those platforms)

@ShadowsFriend
Copy link
Contributor Author

I actually copied that part from obs-studio's FindZLIB.cmake because this was recommended to me as an example and it seemed reasonable to me. I can remove that part if its too hacky for you, especially considering that Dolphin is officially announced to only run on 64 bit systems.

@Sonicadvance1
Copy link
Contributor

We also detect 32bit or 64bit architectures via that check in the main CMakeLists file.
There is a comment in there explaining that it'll break on x32 ABI systems and that if we start supporting that architecture then it will need to be changed.
It's currently the best way to determine what the bitness of the architecture is without adding other checks.

@phire
Copy link
Member

phire commented Oct 5, 2015

We don't need to detect the bitness of the system. /lib should always point to your library, if it doesn't, you are cross compiling, and you have bigger problems and /lib64 isn't the correct place either.

The person who wrote this .cmake file was just paranoid.

@ShadowsFriend
Copy link
Contributor Author

I just removed the bfd dependency from the CMakelists for the OProfile option because it is not needed at all. While OProfile itself depends on it, Dolphin doesn't. And there is no reason to link against a library not needed.

@phire
Copy link
Member

phire commented Oct 6, 2015

@dolphin-emu-bot rebuild

@ShadowsFriend ShadowsFriend changed the title Add FindOprofile.cmake and fix the discovery and linking of OProfile Fix OProfile discovery and drop bfd dependency Oct 6, 2015
@ShadowsFriend
Copy link
Contributor Author

I squashed the three commits into one and changed the title of the PR to better reflect the current proposal.

/usr
/sw
/opt/local
/opt)

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

Adds a cmake module to correctly discover OProfile and adjusts the
corresponding CMakeLists to make use of it. Additionally removes
linking against the bfd library when compiling with OProfile because
Dolphin does not use it.
@ShadowsFriend
Copy link
Contributor Author

Stripped it down to the everything that is needed to find OProfile in /usr as well as in /usr/local. Additionally implemented the requested changes. Thanks for the feedback and sorry for the mess.

@Tilka
Copy link
Member

Tilka commented Oct 11, 2015

lgtm

Tilka added a commit that referenced this pull request Oct 11, 2015
Fix OProfile discovery and drop bfd dependency
@Tilka Tilka merged commit ba20f11 into dolphin-emu:master Oct 11, 2015
@ShadowsFriend ShadowsFriend deleted the master branch October 11, 2015 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants