Skip to content

Conversation

@dongcarl
Copy link
Contributor

Clearly this was meant to be pktinfo_size

@camshaft
Copy link
Contributor

I'm curious to know what platform you're using so we can add it to the CI. Ideally we would be testing for this 😄.

@dongcarl
Copy link
Contributor Author

dongcarl commented Jun 28, 2023

I'm actually on quite a strange platform: gramine.

Basically they LD_PRELOAD glibc and friends and implement a subset of Linux syscalls: https://gramine.readthedocs.io/en/latest/devel/features.html#tcp-ip-and-udp-ip-sockets

Since s2n-quic's feature detection in quic/s2n-quic-platform/build.rs is compilation and target_os-based, all the Linux features (sockopts) were enabled, which means it didn't run in gramine.

I had to patch build.rs like so:

diff --git a/quic/s2n-quic-platform/build.rs b/quic/s2n-quic-platform/build.rs
index 0777e6cf..0b65275f 100644
--- a/quic/s2n-quic-platform/build.rs
+++ b/quic/s2n-quic-platform/build.rs
@@ -6,6 +6,13 @@ use std::{fs::read_dir, io::Error, path::Path, process::Command};
 fn main() -> Result<(), Error> {
     let env = Env::new();

+    const SIMPLE_ENV_VAR: &str = "S2N_SIMPLE_ONLY";
+    println!("cargo:rerun-if-env-changed={}", SIMPLE_ENV_VAR);
+    let is_simple = std::env::var(SIMPLE_ENV_VAR).is_ok();
+    if is_simple {
+        return Ok(())
+    }
+
     for feature in read_dir("features")? {
         let path = feature?.path();
         if let Some(name) = path.file_stem() {

In order to turn off the features.

Thankfully the pktinfo_size change was all that was needed afterwards to make things compile and work (you guys did a great job abstracting away the platform).

Not sure if my build.rs patch is good but perhaps it'd make sense to have some flag that turns off all platform features to test the simple modules in your CI?

@camshaft
Copy link
Contributor

Not sure if my build.rs patch is good but perhaps it'd make sense to have some flag that turns off all platform features to test the simple modules in your CI?

Yeah I think it would be a good idea to have some way to disable all of the features to prevent issues like this. Would you be able to open an issue for it and we can discuss it there?

@camshaft camshaft merged commit 38c4656 into aws:main Jun 28, 2023
@dongcarl
Copy link
Contributor Author

Sure!

@dongcarl
Copy link
Contributor Author

Opened: #1850

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.

2 participants