Upgrade libtcod to 1.16.0-alpha.12 #309
Conversation
Now (optionally) runs bindgen during tcod_sys build; requires (and generates) separate bindings per target
Bit of a kludge, hopefully will find a more permanent solution in the future.
That is, wherever `TCOD_Error` is returned
|
Well, it succeeded on The other ones that fail give
Which make sense, since I didn't pre-generate any of those. I can probably fix that by forcing generation of bindings for those platforms, although it's kind of worthless (as in end-users on those systems will also get that error) without those also being committed back into the repo. |
|
Wow, nice work! |
And also to write out their bindings so they can be included in the repo
|
I'm glad you found it easy to use, @lucanLepus . I'm having a hard time getting the builds to work, though. I have to make changes and then wait until errors show up and then try to fix them, and so it goes over and over. |
(Just use c_int always, like the function that accepts it expects)
|
Getting closer. 😅 Now works for Linux and MSVC Windows builds. Still fails on OSX and GNU Windows builds. |
ilyvion
force-pushed
the
master
branch
4 times, most recently
from
090c649 to
f479d73
Compare
(Also, get a decent grep)
|
Alright, this is where I'm at after a lot of fiddling with the build configuration:
I think I've messed with this enough today, though. 😅 |
|
Only mingw32 build not working at this point. If anyone is willing to look into that, be my guest. Every other target builds and has now got their bindings bundled. Unless 32-bit mingw support is critical, I'd say this PR is ready for merging at this point. |
|
@alexschrod thank you so much for this! It has clearly been a massive effort and I really appreciate you stuck with it! Updating the libtcod version has been requested by many people and none of the current maintainers really had the resources to fully make it happen. I'm happy to merge this as is and then update this in follow-up PRs, but there's few things I'd like to clarify first:
|
|
@tomassedovic I'll do my best to address what needs addressing.
As the
I based my ability on being loose with what changes I could make to the API on exactly that point, in fact. As I mentioned to you in our private conversation as well, I intend to (and have been, to some extent) go through the existing API and improve things where I think improvement is possible, as well as update the wrappers to include more of the things from the native API, and it's probably a good idea to hold off on going to a major version until everything is at least somewhat complete and we're all happy with the way the API looks. We should at the very least bump the minor version before a release, though, for sure.
I would have, except version 1.16 wasn't out of alpha yet. I decided to go with 1.16 because, as the current maintainer of the library said:
I too ran into issues with C++ like what happened in #290 when I tried moving to a version that used it, and wanted to skip that whole spectacle. If necessary, we can hold off on releasing a new version of
I don't know what you are referring to here. If you're talking about this:
Because the
I figured we might as well use the CI to generate the bindings for us, and so that's what I've set up. That way, whenever we need new bindings generated for a new version of libtcod, the CI will generate them for all the supported targets so we don't have to necessarily have all the desired targets available ourselves.
Indeed, it's not. You can see on one of the builds before I did that that libtcod is using features that aren't present in the SDL2 from the package manager. I believe #290 had similar problems; I essentially lifted the code from there to get it working in this PR. |
|
Ooops! Yes of course, we're under 1.0.0 so there's no need to do a major version bump. I don't know what I was thinking. We should still document breaking changes in the release notes to make it easier for folks to upgrade. I'm absolutely open to improving the API before we commit to it. I've seen that note from HexDecimal, but hadn't realised they were talking about a 1.16-alpha. This is fine by me, then. And I was indeed referring to the curl upload command which I misunderstood, sorry! Okay, I am happy to merge this as-is. If you still agree, let me know or feel free to merge it yourself (you should be able to now). |
|
Now I'm just confused. The SDL error is back, but the only thing I changed since my last commit that succeeded was to update the README and non-build related fields in Cargo.toml. |
|
Later versions of Ubuntu will provide later versions of SDL via APT. Try adding
Not sure how well Rust handles this but libtcod would work best if paired with a proper Rust port of SDL rather than being used for things like events through its own API. This might be something to consider later. |
Okay, so... this PR is quite large, and I've made some pretty deep changes to this library, particularly to the build system, so let me just enumerate those real quick:
First and foremost, I've upgraded to libtcod to version 1.16.0-alpha.12. I also did a minor code change, such that those functions/methods that wrap functions that return
TCOD_Errornow returnResult<T, Error>whereErroris my Rust-wrapped error type. I have otherwise not really added any new wrappers or functionality.Next, in the course of my upgrading the bindings, I learned that it's not sound to use the same generated bindings file for every platform. One should generate bindings per platform. I realize this is tedious and can also be a quite the burden on end-users, so I added a feature to
tcod_syscalledgenerate_bindings, and only with that feature enabled will it try to runbindgen. If the feature is not enabled, it will instead look for a pre-generated binding for the given$TARGETand use that instead (and panic otherwise); meaning that it's possible to pre-generate bindings for all the popular targets. I noticed that this project uses both Travis and Appveyor, and so it shouldn't be too difficult to have those generate and commit bindings automatically whenever they change, if desirable. I have not added this (yet, anyway). Since I mainly work on Windows, and have an up and running Ubuntu virtual machine, I have pre-generated bindings forx86_64-pc-windows-msvcandx86_64-unknown-linux-gnu.At this point, I'm curious to see if the CI accepts my code, so let's give it a whirl.