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

Replace rouille with warp #2241

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Saruniks
Copy link
Contributor

@Saruniks Saruniks commented Aug 2, 2024

Part of porting changes from cachepot #1620, paritytech/cachepot#131

@Saruniks Saruniks marked this pull request as draft August 2, 2024 14:38
@Xuanwo
Copy link
Collaborator

Xuanwo commented Aug 2, 2024

Wow, great work! Thanks in advance; Please ping me while you think this PR is ready.

By the way, could you add @montekki as a co-author on this PR? I think it would be great to give them recognition.

@codecov-commenter
Copy link

codecov-commenter commented Aug 2, 2024

Codecov Report

Attention: Patch coverage is 7.54717% with 49 lines in your changes missing coverage. Please review.

Project coverage is 41.09%. Comparing base (0cc0c62) to head (134e1a2).
Report is 71 commits behind head on main.

Files Patch % Lines
src/dist/http.rs 0.00% 26 Missing ⚠️
tests/system.rs 23.52% 13 Missing ⚠️
src/dist/client_auth.rs 0.00% 8 Missing ⚠️
src/util.rs 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #2241       +/-   ##
===========================================
+ Coverage   30.91%   41.09%   +10.18%     
===========================================
  Files          53       55        +2     
  Lines       20112    20857      +745     
  Branches     9755     9844       +89     
===========================================
+ Hits         6217     8571     +2354     
- Misses       7922     8176      +254     
+ Partials     5973     4110     -1863     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Saruniks
Copy link
Contributor Author

Saruniks commented Aug 2, 2024

I'll add montekki as a co-author.

montekki, please let us know if you have any comments on this PR or if you would like to takeover the work.


I'm not sure how to prepare this for a merge. Fixing the merge conflicts and getting the CI to pass was straightforward, but there are a lot of changes, and there's a risk for fallout.
I'll give an update after testing this on my setup.


I wonder what's up with the freebsd tests though.

@Saruniks
Copy link
Contributor Author

Saruniks commented Aug 3, 2024

I did some testing. For some reason sccache_dist scheduler doesn't work properly without SCCACHE_NO_DAEMON=1.

I tested and it even works like this:
SCCACHE_NO_DAEMON=1 sccache-dist scheduler --config /etc/sccache/scheduler.conf &

This is the same reason why freebsd ci doesn't pass. I'll try to fix this issue.

I don't have any useful logs yet.

@Saruniks
Copy link
Contributor Author

Saruniks commented Aug 7, 2024

I found out that I need to create a tokio::Runtime instance manually after call to daemonize, instead o using the #[tokio::main] attribute.

@Xuanwo
Copy link
Collaborator

Xuanwo commented Aug 7, 2024

All unit test for freebsd passed, we only have a No distributed compilations succeeded. Seems the dist-server or dist-client doesn't start correctly on freebsd.

We expect to see something like:

Statistics of second buildtest
  Compile requests                      13
  Compile requests executed              4
  Cache hits                             4
  Cache hits (Rust)                      4
  Cache misses                           0
  Cache hits rate                   100.00 %
  Cache hits rate (Rust)            100.00 %
  Cache timeouts                         0
  Cache read errors                      0
  Forced recaches                        0
  Cache write errors                     0
  Compilation failures                   0
  Cache errors                           0
  Non-cacheable compilations             0
  Non-cacheable calls                    8
  Non-compilation calls                  1
  Unsupported compiler calls             0
  Average cache write                0.000 s
  Average compiler                   0.000 s
  Average cache read hit             0.000 s
  Failed distributed compilations        0

Co-authored-by: Fedor Sakharov <[email protected]>
@Saruniks
Copy link
Contributor Author

Saruniks commented Aug 8, 2024

I fixed freebsd test by creating tokio::Runtime manually instead of using #[tokio::main]:

    let runtime = Runtime::new().context("Failed to create Tokio runtime")?;
    runtime.block_on(async { http_scheduler.start().await })?;

It was a problem that I noticed while testing on ubuntu too.

Co-authored-by: Fedor Sakharov <[email protected]>
Copy link
Collaborator

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @Saruniks, for making this happen, and also thanks to @montekki for the original work. Overall, this PR looks good to me.

Some details inside this PR can be polished in the future.

@Xuanwo
Copy link
Collaborator

Xuanwo commented Aug 8, 2024

cc @sylvestre would you like to take another look?

src/util.rs Outdated
// https://github.com/seanmonstar/reqwest/issues/1328
// https://github.com/briansmith/webpki/issues/54
#[cfg(any(feature = "dist-client", feature = "dist-server"))]
pub fn native_tls_no_sni_client_builder<I, T>(root_certs: I) -> Result<reqwest::ClientBuilder>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Xuanwo I just noticed that native_tls_no_sni_client_builder method might not be needed anymore as seanmonstar/reqwest#1328 was already fixed

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please feel free to remove it, thanks in advance.

Co-authored-by: Fedor Sakharov <[email protected]>
@Saruniks Saruniks marked this pull request as ready for review August 8, 2024 13:44
@sylvestre sylvestre requested a review from glandium August 8, 2024 15:28
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