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

Significant issues compiling on systems with OpenSSL 1.1 #69

Closed
dgunay opened this issue Oct 19, 2020 · 11 comments
Closed

Significant issues compiling on systems with OpenSSL 1.1 #69

dgunay opened this issue Oct 19, 2020 · 11 comments

Comments

@dgunay
Copy link
Contributor

dgunay commented Oct 19, 2020

Hi, I recently tried to compile this on several platforms (Ubuntu 18.04 WSL, Ubuntu 20.04 VM, Alpine 3.12) and had a lot of trouble compiling the code because several dependencies rely on OpenSSL v0.9.X. I followed the typical advice of installing libssl-dev and pkg-config but that didn't seem to make the problem go away.

I was able to successfully build the docker image at least. I was not able to pull the image due to permission issues so I had to build it.

Out of curiosity I attempted to update enough dependencies to stop having compilation issues with outdated OpenSSL dependencies. It eventually worked but some breaking API changes were made along the way (since the dependencies compiled, but then the main crate code stopped compiling).

It would be nice if the project were updated to make things less difficult on newer systems. This would not necessarily be trivial (again due to the necessity of rewriting and testing the code) but would be much appreciated.

I may take a shot at it myself. Let me know if you have any advice or any reasons why maybe this isn't worth pursuing (for all I know I may have just had something silly misconfigured).

@dgunay
Copy link
Contributor Author

dgunay commented Oct 19, 2020

Starting to realize what an undertaking this would be. Serenity's APIs have changed significantly (especially the command building features). A good start would be opening permissions on eu.gcr.io/dom-5-status/dom-5-status so maybe people don't have to build the image themselves.

@dgunay
Copy link
Contributor Author

dgunay commented Oct 19, 2020

For reference I guess: serenity-rs/serenity#394

@dgunay
Copy link
Contributor Author

dgunay commented Oct 19, 2020

I've found a solution, though it isn't pretty: serenity-rs/serenity#394 (comment)

Someone wrote a compatibility branch 2 years ago and made a PR against rust-openssl to support OpenSSL v0.9. While their PR was rejected, it can still be used to successfully compile dominions-5-status on systems with OpenSSL v1.1 by adding this to Cargo.toml:

# Patches dependencies to work with openssl 1.1 correctly
[patch.crates-io]
openssl = { git = "https://github.com/ishitatsuyuki/rust-openssl", branch = "0.9.x" }

This patch allowed me to compile the code on my systems without Docker.

If there is a way to apply this patch automatically on applicable systems (or at least alert the user) that might be useful.

Thanks again for making this, it's really cool.

@djmcgill
Copy link
Owner

Hello! I will open the permissions, sure. Or put it on dockerhub or something.

If you can still build from docker, is there a blocking issue here? I guess it's a massive pain to work on.

Either way good news, I have a WIP branch that's effectively a full rewrite (leaving only the domain structs) that uses tokio 0.2 channels etc to avoid the many issues with the current design
Such as:

  • !start command is not async and is blocking while it PMs the players,
  • duplicate turn pings happen and I can't figure out why, and
  • if the server has an invalid map it captures and never releases TCP connections despite my attempt at adding timeouts and eventually exhausts the pool

Bad news this branch is about a year old and I don't really have huge amounts of motivation to work on it. I will at least push what I have.

Thanks for the kind words.

@dgunay
Copy link
Contributor Author

dgunay commented Oct 19, 2020

Thanks. Not really a blocking issue, I just tend to get obsessed and go down the rabbit hole when I run into issues like that, haha.

I made a PR to get the project to compile with Serenity 0.8 (#70) but it's a real hack-job so far. I think I'll take a look at that branch you're working on and see if they can be integrated.

@djmcgill
Copy link
Owner

Here's my branch: https://github.com/djmcgill/dominions-5-status/tree/rewrite-turn-checker
I suspect we'd have done mostly the same thing. It's not a year old, but is 3 months old, but also as I said I got about halfway through rewriting the turn-checking stuff so that certainly doesn't work.

As for tests, I kind of gave up after a while since the majority of issues I had were to do with interacting with discord instead of db issues. Other than that damn duplicate turn notifications bug! I'd never do this at work of course but, you know how it is with hobby projects. For testing I set up a test discord server and just ran through the commands manually. Hardly ideal, but if you know if a discord integration testing framework I'd love to hear about it! Maybe that could be the next project 🤔

@rustylaz
Copy link

Hey guys, just tried compiling this on a fresh linux install and I got the same issue.

The problem seems to be specifically with OpenSSL 1.1.1, I was able to get it to work by downloading 1.1.0 source and compiling/installing it myself. https://www.spinup.com/installing-openssl-on-ubuntu/ worked for me.

I also tried updating the crates myself, but like you said, API changes led to compile issues. I think the top level culprit is Serenity 0.5.x, it depends on some old crates that manually check for openssl versions up to 1.1.0. Updating to a new version of Serenity should fix this I THINK, but it will require code changes and I'm not familiar enough with rust.

@djmcgill
Copy link
Owner

One day, eventually, I will get around to doing this myself!

@dgunay
Copy link
Contributor Author

dgunay commented Dec 18, 2020

@rustylaz another option is to apply a patch in Cargo.toml: #69 (comment)

@djmcgill
Copy link
Owner

djmcgill commented Feb 22, 2021

Hi both, it only took me 2 months which was honestly less than expected.
WIP PR is #76
HOWEVER note that the turn-checking logic isn't implemented yet.

@djmcgill
Copy link
Owner

Fixed by #76

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants