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

Upgrade and swap of dependencies, huge Refactoring #4

Merged
merged 2 commits into from
Mar 1, 2022
Merged

Conversation

mfelsche
Copy link
Contributor

@mfelsche mfelsche commented Feb 9, 2022

  • Switch to latest stable rust (1.58.1)
  • Switch HTTP framework from rocket to axum as rocket required nightly rust and updating it created conflicts with hubcaps. Also axum is quite new in the tokio ecosystem and is well maintained.
  • move from hubcaps to octocrab for github api support, as using hubcaps with more recent versions of tokio and hyper (used for the http framework as transitive dependency) lead to conflicts which made this app fail to compile. (See Dependency conflict when using hubcaps together with hyper 0.14 softprops/hubcaps#299)
  • Avoid spawning threads where possible and do everything async. This should be a little easier on resources.
  • Fix failing LLVM IR output.
  • Add stderr to output.

I know this is a huge PR and it has lots of stuff smooshed together. But it is an improvement in both code quality and recency of dependencies. It should also give a really small performance improvement. I would appreciate feedback from anyone with a Rust background and welcome all questions from anyone without.

The dependency on my fork of octocrab will be removed, once gist update support is merged upstream and released. I will take care of maintaining that dependency.

From local testing i can verify that the steps for installation and deployment are still the same. Nothing changed on the administrative side here.

All existing functionality still exists.

This PR can be easily verified locally with a rustup installation and a personal GITHUB_TOKEN.

@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Feb 9, 2022
Matthias Wahl and others added 2 commits February 9, 2022 23:32
E.g. go fully async without explicitly using threads. Use axum instead of rocket.
Still WIP.
Signed-off-by: Matthias Wahl <matthiaswahl@m7w3.de>
@SeanTAllen
Copy link
Member

I'm not qualified to review this. Personally, I put faith in you on it @mfelsche. Is there anyone you want to review this?

@SeanTAllen
Copy link
Member

@mfelsche if no one weighs in on this by/during sync on the 15th, are you ok with having this merged sans any other eyes?

@mfelsche
Copy link
Contributor Author

I am totally fine with this being merged without anyone else having a look at this. But then you gotta trust me, that everything is fine with this PR and no one is going to mine bitcoins with this.
I tried to keep the playground the same from a users viewpoint, so running it locally and verifying it does the right thing, should be enough, if noone wants to dive deeper.

@ergl
Copy link
Member

ergl commented Feb 14, 2022

I'm not knowledgeable enough about Rust to comment on the code, but I tried this PR locally and everything seems to work fine.

src/lib.rs Show resolved Hide resolved
static/web.css Show resolved Hide resolved
@ergl
Copy link
Member

ergl commented Mar 1, 2022

This wasn't covered in the sync call, but I think this should be good to merge. On my quick, unexpert review, everything looked fine. Do you agree @mfelsche ?

@SeanTAllen SeanTAllen merged commit 1ae69e6 into main Mar 1, 2022
@SeanTAllen SeanTAllen deleted the upgrades branch March 1, 2022 20:15
@ponylang-main ponylang-main removed the discuss during sync Should be discussed during an upcoming sync label Mar 1, 2022
@SeanTAllen
Copy link
Member

This doesn't work in prod.

@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Mar 1, 2022
@ergl ergl removed the discuss during sync Should be discussed during an upcoming sync label Mar 1, 2022
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

Successfully merging this pull request may close these issues.

4 participants