Sfml formula: sfml 2.1 -> 2.2

[eleweek]

Resolves #35277

Basically: unlike 2.1, sfml 2.2 builds just fine on yosemite, so I created this pull-request. Thanks to @DomT4 for the help with this pull-request!

[DomT4]

You can kill the space here too :). homepage, url and sha are all one solid block unless there's a need for stable do, etc.

[DomT4]

Aside the tiny spacing issue I highlighted, LGTM 👍

[DomT4]

@jacknagel The bot seems to be failing all over the place. Have we broken it?

# Running:

...........................................................................................................................................................................................................................................................................................................................................................FFFFFFFFFFFF.......................................................................................................................................................................................

Finished in 2.352394s, 230.4036 runs/s, 508.8433 assertions/s.

  1) Failure:
PatchingTests#test_patch_array [/usr/local/Library/Homebrew/test/test_patching.rb:102]:
libexec/NOOP was not patched as expected.
Expected "#!/bin/bash\necho NOOP" to not include "NOOP".


  2) Failure:
PatchingTests#test_single_patch_dsl [/usr/local/Library/Homebrew/test/test_patching.rb:47]:
libexec/NOOP was not patched as expected.
Expected "#!/bin/bash\necho NOOP" to not include "NOOP".


  3) Failure:
PatchingTests#test_single_patch_dsl_with_strip [/usr/local/Library/Homebrew/test/test_patching.rb:58]:
libexec/NOOP was not patched as expected.
Expected "#!/bin/bash\necho NOOP" to not include "NOOP".


  4) Failure:
PatchingTests#test_patch_hash [/usr/local/Library/Homebrew/test/test_patching.rb:112]:
libexec/NOOP was not patched as expected.
Expected "#!/bin/bash\necho NOOP" to not include "NOOP".

@eleweek The actual failure here, underneath the above errors is:

==> brew test csfml                          FAILED
Testing csfml
==> /usr/bin/clang test.c -lcsfml-window -o test
==> ./test
./test

dyld: Library not loaded: /usr/local/lib/libsfml-window.2.dylib
  Referenced from: /usr/local/lib/libcsfml-window.2.dylib
  Reason: image not found
Error: csfml: failed
Failed executing: ./test 

Perhaps the dylib has been renamed/reversioned or something?

[jacknagel]

I fixed the unit test failure several hours ago.

[DomT4]

Ah, thanks! Apologies, I thought this PR was more recent than it was,
evidently. Losing track of time.

Sent from OS X. If you wish to communicate more securely my PGP Public
Key is 0x872524db9d74326c.

On 27/12/2014 05:46, Jack Nagel wrote:

I fixed the unit test failure several hours ago.

—
Reply to this email directly or view it on GitHub
#35279 (comment).

[MikeMcQuaid]

Perhaps the dylib has been renamed/reversioned or something?

Yup, we'll need to add/bump the revision in the csfml formula.

[eleweek]

1. There is a newer version of csfml, 2.1, maybe it'll fix the problem?
2. I was unable to build csfml formula on yosemite. this is what cmake says:

CMake Error at cmake/Config.cmake:29 (message): Unsupported version of OS X : 10.10.1

This is easy to fix though, and it was fixed in CSFML master: SFML/CSFML@31ac05d

I wonder, what is the proper solution here. Do I need to provide a patch in formula?

[MikeMcQuaid]

@eleweek Yeh, updating the version seems like a good idea anyway. If you need to patch it on top of that it's probably worth doing too by using https://github.com/LaurentGomila/CSFML/commit/31ac05.patch as a patch URL.

[eleweek]

I've just spend an hour trying to debug a weird issue. It turns out the source(http://www.sfml-dev.org/files/CSFML-2.1-sources.zip) uses \r\n as line terminator(windows style). It turns out this is a big deal for patch utility, and it really wants to see \n unix terminators.

--ignore-whispace doesn't help, but removing \r from the source helps -- after doing this I was able to apply the patch just fine.

Interestingly, adding \r before \n in patch didn't help.

The most easy solution is just strip out \r symbols from source before applying the patch, but can homebrew do this? Maybe I could simply craft a special patch that removes these symbols and then applies the necessary patch?

[MikeMcQuaid]

@eleweek I guess in that case it's probably worth just creating our own patch and linking to the upstream commit.

[eleweek]

Ok! I crafted the necessary patch. What's the best place to put it? I tried gist.github.com, but it strips \r symbols(tried to copy and paste file as well as using python module simplegist). Looks like gist.github issue to me.

[DomT4]

Does that include on the raw. link as well? Normally raw formats things as intended.

[eleweek]

How do I obtain this link? If I press the "raw" button, it gives me a plain text version, but it doesn't contain \r (I checked using hexdump).

Anyway, I figured out a hack, if you upload a file with "\r\r\n" terminators, github automatically replaces "\r\n" with "\n" and you get proper "\r\n" terminators.

The patch applied fine, but it turns out that csfml 2.1 can't be builded against sfml 2.2, so there is a need for at least one more patch.

The fix appear to be less trivial: SFML/CSFML@d6849a3

I'm wondering, what is the proper way to fix this problem:

1. Craft another patch(we can't use the github one due to this separator thing)
2. Fixate a revision, e.g. 848b4d3aaa7646c4742222489416ced48335274f ( current HEAD)
3. Just build from current HEAD, whatever it is.

Personally I like the idea of fixating current HEAD.

[jacknagel]

IIRC git am treats its input as email and converts CRLF -> LF without --keep-cr.

[eleweek]

Do you imply that it properly deal with CRLF in original files? (Patch by default converts CRLF -> LF too, but it seems impossible to disable this behaviour).

Is it possible to use git am for patching using homebrew? If answer is "yes to both", I am going to try it and update the pull-request accordingly.

[MikeMcQuaid]

Is it possible to use git am for patching using homebrew? If answer is "yes to both", I am going to try it and update the pull-request accordingly.

No, unfortunately.

[eleweek]

Hi, I fixated the current HEAD in csfml formula, it now builds ok. I also created an issue in CSFML repository about making a new stable release of csfml.

I like the current solution better than cherry picking multiple patches from master. What do you think?

[MikeMcQuaid]

@eleweek I'd prefer we either cherry-picked patches or just wait for a new release.

[eleweek]

Hm, ok. I'll update the pull-request then.

Would it be possible to use inreplace thing for fixing line terminators(CRLF -> LF)? I tried using it, but to me it looks like inreplace modifies files after normal patches are applied.

I'm trying to find the simplest way to deal with CRLF issue(see above discussion).

[MikeMcQuaid]

@eleweek In this case I'd just try and get this stuff fixed upstream and wait for a future release 😭

[eleweek]

Well, I spent a few hours figuring out how to make a patch that applies to files with CRLF and put it on gist.github.com, so I can do it.

Do you think it would be a bad idea if I cherry pick two patches, convert them, and then put them on gist? (And of course update the formula accordingly)

[MikeMcQuaid]

That seems fine to me 👍

[mantognini]

I love to see people using SFML and making it progress further! But please consider putting me in the loop. It can be useful for everybody I think, especially since the SFML formula is not that simple and probably wrong as I mentionned it long ago.

Please also read the issue description on SFML tracker for more details: SFML/SFML#620.

[MikeMcQuaid]

I love to see people using SFML and making it progress further! But please consider putting me in the loop. It can be useful for everybody I think, especially since the SFML formula is not that simple and probably wrong as I mentionned it long ago.

Please also read the issue description on SFML tracker for more details: SFML/SFML#620.

You can get in the loop by following these issues, everything here is happening in the open. If you have any specific, reproducible issues we're happy to try and resolve them but it's not really helpful to say it's "probably wrong" without giving any reproduction instructions or specifics (or a pull request).

[mantognini]

You cannot expect people to follow on everything going on everywhere... There's only 24h in a day and we all have much to do. Anyway, I've explain the issue with the current formula in the two links I gave. If you follow them you'll also find a link to a SO question that I strongly encourage people to read.

[MikeMcQuaid]

@mantognini I read all the links. Nowhere have you given specific, reproducible errors with this formula. I don't expect people to follow everything everywhere but I do expect maintainers of software to be better at filing bug reports.

[mantognini]

@MikeMcQuaid Saying that is not really fair... What about

When built with CMake, SFML will automatically use those binaries unless specified otherwise with proper CMake flags. So the binaries (and header files too) of those libraries won't necessarily come from brew itself even if they are installed.

and in our issue description:

• all deps come from homebrew, extlibs directory is ignored
• playing with CMake variable should be enough for that, no need to edit the script

I think this describe well the issue, but if you have any further question I'd be happy to answer them.

Also, the SO question seems clear enough to me.

[MikeMcQuaid]

@mantognini Ok, to be more explicit:

• tell me what commands I can run on my local machine to produce an error. Step-by-step reproduction instructions: a command to run, what you expect the output to be, what the actual output is.
• instead of calling our formula naive consider actually submitting a pull request to improve it.

[MikeMcQuaid]

Also:

• given we're just running cmake consider fixing your CMake buildsystem so it picks up dependencies from the PATH.

[DomT4]

If upstream feel particularly strongly than the Homebrew formula is "naive" and "probably wrong", we could blacklist the formula and enforce a requirement that people go upstream for it. Simply telling Homebrew it is screwing things up is a recipe for Homebrew to then go "Well, we don't want to provide users a crap experience, so if nobody with the required understanding of SFML is able/willing to help, we're just going to shove people towards upstream where that support does seemingly exist".

"Here's how this could be fixed, here's what I can do to help, here's what upstream can do to help, here's what Homebrew can do to help" will get you a trillion times further than "You've broken it". Tons of people around here love to help, especially the maintainers, just try to make it a little easier for them to do so please 😉.

I'll take a look at the formula later today, but it sounds like long-term there needs to be some work done by upstream as well. Perhaps some mutual ground can be reached.

[mantognini]

Perhaps some mutual ground can be reached.

I hope so! I believe there are sadly some misunderstandings here. SFML team is willing to provide a good user experience with brew (that's why we have it on our TODO list in the first place). I'm not asking you to do so but if you were to follow SFML development closely you would know that we are a small, volunteer and unpaid team (especially when it comes to OS X support) and therefore we don't have the manpower to tackle every bugs or features quickly.

I'm sorry that you interpreted my words as contemptuous; it was at no time my intention - probably that some words I used have different connotations in English and in my mother language. I just wanted to let you guys know that the current implementation is not perfect. Hopefully, someone with more free time than I could find the inspiration to fix things.

Now, of course work has to be done on both sides. There's no question about that. But you have to understand that SFML is not targeted to OS X and specifically brew build system. As such, SFML cannot ensure that it will build nicely out of the box for every system out there. We try of course to make it convenient for the most standard build system.

However, we have already improved the system a bit with SFML/SFML@ba1488e. Beside that, we're also planning to drop sndfile for licensing reason and that changes things for both of us.

@MikeMcQuaid, I don't have a step by step procedure to give you – I don't have the luxury to have enough free time to work on that – but I'm pretty sure that installing SFML through homebrew, then building a very simple app and using lldb target modules list will show that the binary are not loaded from brew cellar but from SFML extlibs folder. (That the major bug I think.)

[DomT4]

we are a small, volunteer and unpaid team

So is Homebrew, alas. Jack and Mike are the maintainers you see in most PRs, but I don't think Homebrew is paying anyone's bills, sadly, despite the time and dedication it requires. There's plenty of mutual empathy around having small teams working extremely hard for little financial return. Homebrew relies heavily on the wider community and people with interests in the formula involved to drive changes and new ideas and fix existing ideas, which is why it tries to get people who love x piece of software to help Brew love it as well.

But you have to understand that SFML is not targeted to OS X and specifically brew build system.

This is perfectly reasonable. A lot of the more popular tools we ship from Brew are actually primarily Linux tools. CMake isn't my favourite build system in the world, but I think it's doable for you to still ship your normal package but perhaps write into the CMakeList an option where people can turn off the use of the bundled packages, and if they do, SFML scans the $PATH for the deps instead? That could be a compromise that works for everyone. It allows you to not break up your ideal build system, but it allows package managers to just flip the one switch on not relying on the bundled deps.

[mantognini]

perhaps write into the CMakeList an option where people can turn off the use of the bundled packages

Ha! Here is the misunderstanding I guess because we already have that. 😉

The manually switch is a bit tedious, I agree, but should work fine as it's the standard cmake way of dealing with dependencies such as -DFREETYPE_LIBRARY=/usr/local/Cellar/freetype/2.4.11/lib/libfreetype.6.dylib. See what was said there: SFML/SFML#424 (comment)

[DomT4]

Apologies, I stealth edited my comment shortly after posting to specify a desire to move to a place where packagers can just flip one switch, i.e. -DBUNDLEDDEPS=0 or similar to turn off the bundled deps and force CMake to look along the $PATH for them instead. Having one switch instead of 10 or so is a lot nicer and less prone to breaking 😉

[mantognini]

This was already suggested but I feel it goes against cmake philosophy. Furthermore it adds a lot of stuff in the script that needs to be maintained in the future. This looks like a heavy burden only to make something we have to write once beautiful.

[DomT4]

Is there a way to stop it automatically setting the rpath of everything? I compiled the examples shipped and immediately ran into:

    @rpath/libsfml-audio.2.2.0.dylib (compatibility version 2.2.0, current version 2.2.0)
    @rpath/libsfml-graphics.2.2.0.dylib (compatibility version 2.2.0, current version 2.2.0)
    @rpath/libsfml-window.2.2.0.dylib (compatibility version 2.2.0, current version 2.2.0)
    @rpath/libsfml-system.2.2.0.dylib (compatibility version 2.2.0, current version 2.2.0)

Welp. I could change them manually, but that's a bit of a nightmare.

If there's a better simple test I could be doing, feel free to point me towards one and I'll run with that instead.

[mantognini]

I don't know exactly what you've done and what should be done, but someone managed to do it just a few hours ago. Maybe you two should work together? :-)

Here is the relevent discussion: http://en.sfml-dev.org/forums/index.php?topic=17136.msg123352#msg123352

[mantognini]

BTW, the @rpath thing is a good thing in fact. It play very nicely when you create a bundle app and include the libraries in it (which is done automatically with the Xcode templates we provide).

[ludvichzme]

Hey ! I come from the discussion on the sfml forum.
I've made some tests changing the CMakeLists to disable using the extlibs which are shipped with the sfml.
As saying in the post, if we just remove them, it seems that the compiler just fallback to the /usr/local/lib to find the required libraries. That might be a solution to our problems.

[DomT4]

Patching it out and explicitly specifying flags was pretty much what I had in mind as well, as it happens. I fell asleep on this earlier because winter makes me feel like a 65 year old, but I've pushed now in #35479 😉. If someone from upstream can run through the changes and test to make sure the problem is actually solved, that'd be great.

[eleweek]

So, what the status of this pull-request? @DomT4 opened another pull request that fixed some other issues. Do I need to close this one?

[DomT4]

If you remove the sfml changes from this PR, but leave the csfml changes in, we can merge this after the sfml issue is dealt with, I imagine.

[DomT4]

@eleweek If you'd like to make this PR csfml-only and then rebase it against the master, we should be okay here providing the maintainers don't object to using a commit reference temporarily as a solution till new upstream release.

[eleweek]

@DomT4 I am going to wait for this to be closed: SFML/CSFML#50

Would this be ok? It looks like CSFML maintainers are going to release a new version, so my pull request is going to be a lot simpler

[DomT4]

Sure, If you're happy to wait, that sounds good. Feel free to close this in that case.

[eleweek]

Ok, so the new version of CSFML is released and I created a new pull-request with only CSFML-related changes: #38036