-
Notifications
You must be signed in to change notification settings - Fork 21
Check nginx' version if build's name starts with "nginx" #89
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
base: main
Are you sure you want to change the base?
Conversation
|
✅ All required contributors have signed the F5 CLA for this PR. Thank you! |
|
I have hereby read the F5 CLA and agree to its terms |
bavshin-f5
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for delay, did not get a chance to review this before holidays.
|
|
||
| println!("cargo::rustc-check-cfg=cfg(ngx_ssl_cache)"); | ||
| println!("cargo::rustc-check-cfg=cfg(ngx_ssl_client_hello_cb)"); | ||
| println!("cargo::rerun-if-env-changed=DEP_NGINX_VER_BUILD"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DEP_NGINX_BUILD or DEP_NGINX_VERSION, see https://docs.rs/nginx-sys/0.5.0/nginx_sys/#version-and-build-information.
These environment variables are generated from cargo::metadata={name}=... in the nginx-sys buildscript, using DEP_{package.links}_{name} format. I'm afraid it's too late to change the metadata names to match the macros in nginx.h.
| println!("cargo::rerun-if-env-changed=DEP_NGINX_VERSION_NUMBER"); | ||
| if let Ok(version) = env::var("DEP_NGINX_VERSION_NUMBER") { | ||
| let version: u64 = version.parse().unwrap(); | ||
| let build: String = env::var("DEP_NGINX_VER_BUILD").unwrap_or("any()".to_string()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.unwrap_or_default().
I'd also prefer to save the check state as a bool and use in subsequent conditionals, allowing future version checks for different projects:
let is_nginx = env::var("DEP_NGINX_VERSION").unwrap_or_default().starts_with("nginx/");
if is_nginx && version >= ...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that requires an additional change in nginx-sys ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, everything I suggested here works with the current release of nginx-sys.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, sounds good to me, thanks.
Proposed changes
The proposed changes adopt the application make a check of nginx' version if the build's name start with "nginx".
These changes sponsored by: https://tipi.work/ - this's the part of the change, can't be removed.
Checklist
Before creating a PR, run through this checklist and mark each as complete:
README.mdand/orCHANGELOG.md).