Skip to content

port login server to rust #1717

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

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

Conversation

nornagon-openai
Copy link
Collaborator

replace the python login server with rust.

codex ported this, and I haven't thoroughly checked the logic, but it does work.

Comment on lines +21 to +28
hyper = { version = "1", features = ["http1", "server"] }
hyper-util = { version = "0.1", features = ["tokio"] }
open = "5"
url = "2"
base64 = "0.22"
sha2 = "0.10"
rand = "0.8"
http-body-util = "0.1"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Having to pay the tax of embedding an entire HTTP server is the reason I had been avoiding this to date...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

tax

what kind of tax?

IMO depending on python is a much larger tax than shipping a few extra KB.

- 238K  libcodex_login.rlib
+ 562K  libcodex_login.rlib

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alpha sort this list, please.

Copy link
Collaborator

@bolinfest bolinfest left a comment

Choose a reason for hiding this comment

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

The original version of the Python was one giant file to facilitate running it with python -C. Since we no longer have that constraint, can you please refactor this?

Comment on lines +21 to +28
hyper = { version = "1", features = ["http1", "server"] }
hyper-util = { version = "0.1", features = ["tokio"] }
open = "5"
url = "2"
base64 = "0.22"
sha2 = "0.10"
rand = "0.8"
http-body-util = "0.1"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Alpha sort this list, please.

}
}

fn serde_urlencode(params: &[(String, String)]) -> String {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this named serde_urlencode()?

Ok(())
}

#[allow(clippy::unwrap_used)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this using unwrap() instead of forwarding the Err variant?

}

// Public entry point used by lib.rs
pub async fn run_login_server(codex_home: &Path) -> std::io::Result<()> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please list the pub entrypoint before the helper functions.

use crate::CLIENT_ID;
use crate::TokenData;

const REQUIRED_PORT: u16 = 1455;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would preserve this client from the Python: # Required port for OAuth client.


const REQUIRED_PORT: u16 = 1455;
const URL_BASE: &str = "http://localhost:1455";
const DEFAULT_ISSUER: &str = "https://auth.openai.com";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where is DEFAULT_CLIENT_ID?

None
}

const LOGIN_SUCCESS_HTML: &str = include_str!("static/success.html");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Put this up top?

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