search

tomassedovic / tcod-rs

Rust bindings for libtcod 1.6.3 (the Doryen library/roguelike toolkit)
Do What The F*ck You Want To Public License
229 stars 45 forks source link

Upgrade libtcod to 1.16.0-alpha.12 #309

Open ilyvion opened 4 years ago

ilyvion commented 4 years ago

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_Error now return Result<T, Error> where Error is 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_sys called generate_bindings, and only with that feature enabled will it try to run bindgen. If the feature is not enabled, it will instead look for a pre-generated binding for the given $TARGET and 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 for x86_64-pc-windows-msvc and x86_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.

ilyvion commented 4 years ago

Well, it succeeded on x86_64-pc-windows-msvc. That's something. 😅 Looks like the issue is an outdated version of SDL2 on Linux.

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.

lucanLepus commented 4 years ago

Wow, nice work! just had a quick build and test on my Arch Linux and everything seems to work nicely. I honestly did not think it would be possible to do such a major update on tcod-sys so painless for end-users.

ilyvion commented 4 years ago

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.

ilyvion commented 4 years ago

Getting closer. 😅 Now works for Linux and MSVC Windows builds. Still fails on OSX and GNU Windows builds.

ilyvion commented 4 years ago

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. 😅

ilyvion commented 4 years ago

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.

ilyvion commented 4 years ago

Merging this PR should resolve #308 and resolve #297.

tomassedovic commented 4 years ago

@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:

ilyvion commented 4 years ago

@tomassedovic I'll do my best to address what needs addressing.

Since this does bring breaking changes to the tcod API, we'll need to bump the major version of tcod-rs too before we publish it on crates.io. We'll also need to document (I've noticed the returning Result in some functions, FontLayout and FontType being bitflags now and input::KEY being moved to input::EventFlags::Key, we'll need to audit the lib for anything else).

As the tcod crate is currently on version 0.15, semantic versioning does not require a major version bump. In fact, the README clearly states as much:

This project follows Semantic Versioning. Since we're under 1.0.0 anything goes. The API can change at any time.

Indeed, it probably should change! If you have better ideas on how it make it safer or more familiar to Rust developers, please let us know.

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.

Why are we using an alpha version of libtcod? I would have expected to use a "fully released" version instead.

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:

The latest versions of libtcod had their C++ dependencies removed and can now be compiled without any C++ sources. You'll only need C99 support from the tool-chain.

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 tcod until libtcod 1.16 stabilizes, although I have no idea when that'll happen, obviously.

Why are we downloading the tcod-sys bindings in the travis.yml file since we're generating them a line ahead?

I don't know what you are referring to here. If you're talking about this: curl -F "file=@./tcod_sys/${BINDINGS_FILE}" https://file.io/ | $GREP -Po '"link":"\K[^"]+', that's actually an upload, not a download. The reason it says "$BINDINGS_FILE download" is because the result of that call will print a link to the CI output that can be used to download the bindings that were generated. Ideally, the bindings file generated would've been declared as an artifact and hosted somewhere more... permanent, but Travis doesn't support artifacts like Appveyor does, so I found a different way to get a hold of it after it was generated. More details in the next answer.

Why are we downloading LLVM on Windows now?

Because the tcod_sys builds are ran with bindgen during CI builds and bindgen requires LLVM. It's also being installed on Linux and Mac; except in those cases through apt and brew, respectively. As I mentioned in my original PR message,

I added a feature to tcod_sys called generate_bindings, and only with that feature enabled will it try to run bindgen. If the feature is not enabled, it will instead look for a pre-generated binding for the given $TARGET and use that instead (and panic otherwise); meaning that it's possible to pre-generate bindings for all the popular targets.

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.

Why are we downloading SDL in linux_script.sh? We're installing libsdl2-dev in linux_install.sh, is that not enough?

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.

tomassedovic commented 4 years ago

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).

ilyvion commented 4 years ago

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.

HexDecimal commented 3 years ago

Later versions of Ubuntu will provide later versions of SDL via APT. Try adding dist: bionic or dist: focal to the TravisCI config file and check the installed version of SDL to make sure it's version 2.0.5 or later. If you get error: ‘SDL_PIXELFORMAT_RGBA32’ undeclared (first use in this function) then you included older SDL headers while building libtcod.

1.16-alpha.x is the most stable version of libtcod as far as bug-fixes are concerned. The alpha tag refers to the context API and other recently added features. Only functions documented as being added in libtcod 1.16 are unstable.

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.