-
Notifications
You must be signed in to change notification settings - Fork 74
feat!: add nginx-src crate with vendored nginx sources #160
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: master
Are you sure you want to change the base?
Conversation
We had an inconvenient expectation that the nginx binary is installed to the configured location and the default prefix is writable. Instead of that, we should take the same approach as perl tests: * Create a temporary prefix and pass it to nginx. * Use binary from the build dir and allow overriding it with environment variable.
Ensure that nginx-sys/vendored is enabled and limit targets to Linux x86_64.
9744571
to
4b7fd2c
Compare
I think that's good enough to start looking. The only remaining item is README.md update. |
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.
Pull Request Overview
Adds a new nginx-src
crate to vendor NGINX sources and updates the existing nginx-sys
build to use it, refactors test harness for finding and running a local NGINX binary, and cleans up the old vendored.rs
logic.
- Introduces
nginx-src
submodule and crate for downloading, verifying, and building NGINX sources - Switches
nginx-sys
’svendored
feature to depend onnginx-src
instead of in-repovendored.rs
- Updates tests to locate an NGINX binary via environment variables and cleans up deprecated test build script
Comments suppressed due to low confidence (4)
nginx-src/src/verifier.rs:28
- There is no
fs::exists
function returning aResult
; you should check existence withif !gnupghome.exists() { ... }
(no?
needed).
if !fs::exists(&gnupghome)? {
build.rs:57
- Cargo build-script directives use a single colon (
cargo:...
), not double; change"cargo::rerun-if-env-changed=..."
to"cargo:rerun-if-env-changed=..."
.
println!("cargo::rerun-if-env-changed=DEP_NGINX_BUILD_DIR");
tests/nginx.conf:19
- [nitpick] Commenting out
include mime.types
alters the test NGINX configuration; ensure tests don’t rely on MIME definitions or explicitly provide what’s needed.
# include mime.types;
See #85 for rationale. Also "solves" all the issues with the dependency URLs, bad signatures etc.
Proof of concept. I'm planning to restore the download code and make it strictly opt-in before I mark this as ready for review.