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

add std,alloc feature flag to support no_std build #111

Merged
merged 4 commits into from
Jan 3, 2025

Conversation

JyJyJcr
Copy link
Contributor

@JyJyJcr JyJyJcr commented Dec 26, 2024

Proposed changes

add features std, alloc to prepare for no_std build support.

  • alloc feature means memory allocation used. Implementation differs depending on std feature:
    • with std: use std crate components. (e.g. std::string::String)
    • without std: use alloc crate components. (e.g. alloc::string::String)
  • std feature means std crate components used. Currently the only difference between std and alloc is impl of std::error::Error (core::error::Error is not in Rust 1.79.0), which is not used.

alloc is the transitional feature: while it enables no_std build, it allows memory allocation using std::alloc::System internally. We can gradually replace alloc crate components by nginx-based ones.

For these features, many std imports are replaced by core ones.

Related issue (enhancement): #109

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have written my commit messages in the Conventional Commits format.
  • I have read the CONTRIBUTING doc
  • I have added tests (when possible) that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto master
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

@bavshin-f5
Copy link
Member

Thanks!
Looks similar to what I intended to do, I'll take a closer look next week when I'm back from vacation.

One comment: I'd prefer to remove String from nginx-sys completely instead of introducing the alloc feature.
Display impl commit in #113 supposed to be a first step towards that, another step is to rewrite nginx_sys::add_to_ngx_table like in bavshin-f5@ae2ee99, and the last step is to remove remaining redundant conversions.

JyJyJcr added a commit to JyJyJcr/ngx-rust that referenced this pull request Dec 27, 2024
follow bavshin-f5's comment: nginx#111 (comment)
copied implementation:
Display for ngx_str_t, and test <= bavshin-f5/ngx-rust@3f054e3
add_to_ngx_table <= bavshin-f5/ngx-rust@ae2ee99
@JyJyJcr
Copy link
Contributor Author

JyJyJcr commented Dec 27, 2024

Thank you for the advice.
I replaced them by your implementation bavshin-f5/ngx-rust@3f054e3, bavshin-f5/ngx-rust@ae2ee99 for the moment.
nginx-sys is now alloc-free.

JyJyJcr added a commit to JyJyJcr/ngx-rust that referenced this pull request Dec 27, 2024
follow bavshin-f5's comment: nginx#111 (comment)
copied implementation:
Display for ngx_str_t, and test <= bavshin-f5/ngx-rust@3f054e3
add_to_ngx_table <= bavshin-f5/ngx-rust@ae2ee99
JyJyJcr added a commit to JyJyJcr/ngx-rust that referenced this pull request Jan 1, 2025
follow bavshin-f5's comment: nginx#111 (comment)
copied implementation:
Display for ngx_str_t, and test <= bavshin-f5/ngx-rust@3f054e3
add_to_ngx_table <= bavshin-f5/ngx-rust@ae2ee99
Copy link
Member

@bavshin-f5 bavshin-f5 left a comment

Choose a reason for hiding this comment

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

The comments for Request::headers_in_iterator/Request::headers_out_iterator need to be updated.


Please, squash all the fixup commits and clean up the history.
I believe this PR can be reorganized into 3-4 commits with distinct logical changes. For example,

  • Replacing std with core and alloc
  • Rework of the header iterator
  • Removal of the remaining String uses
  • Feature flags and documentation

nginx-sys/src/lib.rs Outdated Show resolved Hide resolved
nginx-sys/README.md Outdated Show resolved Hide resolved
nginx-sys/Cargo.toml Outdated Show resolved Hide resolved
src/log.rs Outdated Show resolved Hide resolved
src/http/request.rs Outdated Show resolved Hide resolved
src/http/request.rs Outdated Show resolved Hide resolved
src/core/string.rs Show resolved Hide resolved
src/http/module.rs Outdated Show resolved Hide resolved
@JyJyJcr
Copy link
Contributor Author

JyJyJcr commented Jan 1, 2025

Thank you for the reviewing.

I'll fix the points one by one, and after that do squash.

@JyJyJcr
Copy link
Contributor Author

JyJyJcr commented Jan 2, 2025

chores:

  • NGINX part of workflows has been broken in some point. It seems unrelated to this PR.
  • I've added thumbs up emoji to mark as completed (in my perspective), avoiding annoying you by many short messages.

@bavshin-f5
Copy link
Member

* NGINX part of workflows has been broken in some point. It seems unrelated to this PR.

Worker processes crash when running awssig example code:

# got: '2025/01/02 13:13:30 [alert] 5687#5687: worker process 5688 exited on signal 6 (core dumped)'

That's the only example that uses the header iterators, the crash is likely in Iterator::next implementation.

src/http/request.rs Outdated Show resolved Hide resolved
src/http/request.rs Outdated Show resolved Hide resolved
@JyJyJcr
Copy link
Contributor Author

JyJyJcr commented Jan 2, 2025

commit squashed.

Copy link
Member

@bavshin-f5 bavshin-f5 left a comment

Choose a reason for hiding this comment

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

Almost ready, just a couple more issues.

98cb85d and 0174f10 fail to build. Please, consider reordering the commits as shown below to make each commit buildable:

  1. fix: refactor NgxListIterator to remove String usage
  2. fix: replace other String usage by bavshin-f5's impl
    (use alloc::string::ToString addition in the test code should be moved to the no_std commit)
  3. chore: add Cargo features
  4. fix: replace std to core and add config out

examples/Cargo.toml Show resolved Hide resolved
src/http/request.rs Outdated Show resolved Hide resolved
src/http/request.rs Outdated Show resolved Hide resolved
src/core/string.rs Outdated Show resolved Hide resolved
@bavshin-f5 bavshin-f5 added this to the 0.5.0 milestone Jan 2, 2025
@bavshin-f5 bavshin-f5 linked an issue Jan 3, 2025 that may be closed by this pull request
@bavshin-f5 bavshin-f5 merged commit c3c31ec into nginx:master Jan 3, 2025
9 of 10 checks passed
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.

Support no_std build
2 participants