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

Relaxed cmake installation rules regarding OS X framework dependencies #767

Merged
merged 1 commit into from Feb 12, 2015

Conversation

Projects
None yet
5 participants
@mantognini
Member

mantognini commented Jan 8, 2015

This PR should ease the creation of a homebrew formula. It does not completely supersedes #620 since Xcode templates are not yet (tested and) compatible with brew file hierarchy. But I don't want to slow down the development of the SFML formula for this extra and complex feature.

Related to #620 and Homebrew/legacy-homebrew#35479

NB: #757 would have to fix some conflicts with this patch but that's not a big deal.

@mantognini mantognini added this to the 2.3 milestone Jan 8, 2015

@eXpl0it3r eXpl0it3r removed the s:unassigned label Jan 8, 2015

@DomT4 DomT4 referenced this pull request Jan 10, 2015

Closed

sfml 2.2 #35479

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Jan 10, 2015

Member

This is a minor patch (only +6/-3), anyone feels reviewing it? That way I could work on 757. :-)

Member

mantognini commented Jan 10, 2015

This is a minor patch (only +6/-3), anyone feels reviewing it? That way I could work on 757. :-)

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Jan 10, 2015

Member

Let's assume we use Freetype but skip Soundfile. Freetype won't be installed? Shouldn't there be separate checks rather than "all or none"?

Member

MarioLiebisch commented Jan 10, 2015

Let's assume we use Freetype but skip Soundfile. Freetype won't be installed? Shouldn't there be separate checks rather than "all or none"?

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Jan 10, 2015

Member

That's a valid concern. In theory, installing just what's needed would be better better but in practice people would either use the extlibs folder for everything or delete it and let cmake automatically find and use their own binaries.

In any case, those lines will be rewritten very soon with the no_libsndfile_* branches so even if it's not really nice now, I can make it better then. So others' opinions are of interest to me.

Member

mantognini commented Jan 10, 2015

That's a valid concern. In theory, installing just what's needed would be better better but in practice people would either use the extlibs folder for everything or delete it and let cmake automatically find and use their own binaries.

In any case, those lines will be rewritten very soon with the no_libsndfile_* branches so even if it's not really nice now, I can make it better then. So others' opinions are of interest to me.

@DomT4

This comment has been minimized.

Show comment
Hide comment
@DomT4

DomT4 Jan 13, 2015

Going to be annoying and leave a comment here so I can keep track of when this is merged in so we can go on the brew formula updates.

DomT4 commented Jan 13, 2015

Going to be annoying and leave a comment here so I can keep track of when this is merged in so we can go on the brew formula updates.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Jan 13, 2015

Member

@DomT4: You don't have to comment on an issue to track it. Just hit the "Subscribe" button on the right side of the page.

Member

MarioLiebisch commented Jan 13, 2015

@DomT4: You don't have to comment on an issue to track it. Just hit the "Subscribe" button on the right side of the page.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Jan 13, 2015

Member

I think it was a nice way to say 'up'. :)

others' opinions are of interest to me.

Don't be shy, I don't (usually) bite. 😜

Member

mantognini commented Jan 13, 2015

I think it was a nice way to say 'up'. :)

others' opinions are of interest to me.

Don't be shy, I don't (usually) bite. 😜

@DomT4

This comment has been minimized.

Show comment
Hide comment
@DomT4

DomT4 Jan 13, 2015

Just hit the "Subscribe" button on the right side of the page.

It was 5:40am, this completely slipped my mind. Apologies 😉

DomT4 commented Jan 13, 2015

Just hit the "Subscribe" button on the right side of the page.

It was 5:40am, this completely slipped my mind. Apologies 😉

@i8degrees

This comment has been minimized.

Show comment
Hide comment
@i8degrees

i8degrees Jan 14, 2015

Hi,

I doubt that it will much matter as you state that the sndfile framework will soon vanish, but for whatever it is worth … I agree with Mario, because of the following use case that I’ve had to do so on occasion with with homebrew packages — slipping in a custom patched version of a dependency (usually with the hope that the issue is eventually fixed upstream).

Cheers,
Jeffrey Carpenter i8degrees@gmail.com

i8degrees commented Jan 14, 2015

Hi,

I doubt that it will much matter as you state that the sndfile framework will soon vanish, but for whatever it is worth … I agree with Mario, because of the following use case that I’ve had to do so on occasion with with homebrew packages — slipping in a custom patched version of a dependency (usually with the hope that the issue is eventually fixed upstream).

Cheers,
Jeffrey Carpenter i8degrees@gmail.com

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Jan 16, 2015

Member

@mantognini So should I add this to my mergelist?

Member

eXpl0it3r commented Jan 16, 2015

@mantognini So should I add this to my mergelist?

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Jan 21, 2015

Member

@eXpl0it3r Actually, having thought a bit more about it, I think I'd rather make a proper fix and follow @MarioLiebisch / @i8degrees on this; I'll also update the Xcode template and make sure not to break them, even if it's only for a few commits until no_libsdnfile is merged. I'll post back here soon when it's ready.

Member

mantognini commented Jan 21, 2015

@eXpl0it3r Actually, having thought a bit more about it, I think I'd rather make a proper fix and follow @MarioLiebisch / @i8degrees on this; I'll also update the Xcode template and make sure not to break them, even if it's only for a few commits until no_libsdnfile is merged. I'll post back here soon when it's ready.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Jan 28, 2015

Member

Actually I remember why I didn't updated the Xcode template in the first place: the install script (in xcode, not cmake) need serious fix based on install_name_tool to make sure the dependencies can be loaded from the resource directory of the bundle. If some users really want that I'll work on it, otherwise I'll focus on important features.

@eXpl0it3r @MarioLiebisch @i8degrees @DomT4 This PR has been updated to reflect what has been said above. Please give it a second pair of 👀, thanks.

Member

mantognini commented Jan 28, 2015

Actually I remember why I didn't updated the Xcode template in the first place: the install script (in xcode, not cmake) need serious fix based on install_name_tool to make sure the dependencies can be loaded from the resource directory of the bundle. If some users really want that I'll work on it, otherwise I'll focus on important features.

@eXpl0it3r @MarioLiebisch @i8degrees @DomT4 This PR has been updated to reflect what has been said above. Please give it a second pair of 👀, thanks.

@mantognini mantognini self-assigned this Jan 28, 2015

@DomT4

This comment has been minimized.

Show comment
Hide comment
@DomT4

DomT4 Jan 28, 2015

Not a CMake expert by any means, but this looks like it'd still work well with the Brew proposals. No complaints from me.

DomT4 commented Jan 28, 2015

Not a CMake expert by any means, but this looks like it'd still work well with the Brew proposals. No complaints from me.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Feb 2, 2015

Member

Thanks @DomT4 for the review. Any other opinion? :-)

Member

mantognini commented Feb 2, 2015

Thanks @DomT4 for the review. Any other opinion? :-)

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Feb 11, 2015

Member

Since no one complained, I guess we can merge this. :-)

Member

mantognini commented Feb 11, 2015

Since no one complained, I guess we can merge this. :-)

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Feb 11, 2015

Member

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

Member

eXpl0it3r commented Feb 11, 2015

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

Relaxed cmake installation rules regarding OS X framework dependencies
No longer install sndfile and/or freetype frameworks if SFML is built libraries not from the extlibs folder.
Related to #620 and Homebrew/legacy-homebrew#35479

Xcode templates are not updated yet to reflect that change since it involve toying with `install_name_tool` and is quite complex.

@eXpl0it3r eXpl0it3r merged commit 9f2aecf into master Feb 12, 2015

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Feb 12, 2015

Member

Rebased onto master and merged in 9f2aecf.

Member

eXpl0it3r commented Feb 12, 2015

Rebased onto master and merged in 9f2aecf.

@eXpl0it3r eXpl0it3r deleted the feature/upgrade_cmake_install_rules branch Feb 12, 2015

@mantognini mantognini removed their assignment Apr 30, 2015

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