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

Platform support and auto scripts improvements #124

Merged
merged 5 commits into from
Feb 8, 2025

Conversation

bavshin-f5
Copy link
Member

@bavshin-f5 bavshin-f5 commented Jan 25, 2025

The whole thing now kind of works on Windows1. Also on NetBSD, OpenBSD and Illumos.
Getting closer to the "Tested OS and platforms" of nginx.

In the course of debugging, examples/config accumulated too many subtle details so I split it into a reusable examples/auto/rust. The idea is to drop the file into your project, copy config.make and write a minimal config.

TODOs:

  • Documentation updates
  • Copyright header in auto/rust to allow redistribution.
  • Decide if it's worth renaming NGX_OBJS to NGINX_BUILD_DIR

Things to address later:

  • Detect NGX_ALIGNMENT redefined via --with-cc-opt and add a compile-time assertion for insufficient alignment, Use CFLAGS from the Makefile #118
  • Figure out how to disentangle configure/make commands from nginx-sys/build/vendored.rs
  • Windows CI?

Footnotes

  1. with a couple of patches: nginx#457 and sed -i -e 's/^\(#define _WIN32_WINNT\).*/\1 0x0600/' src/os/win32/ngx_win32_config.h

@xeioex
Copy link
Contributor

xeioex commented Jan 25, 2025

Copyright header in auto/rust to allow redistribution.

Sound good.

Decide if it's worth renaming NGX_OBJS to NGINX_BUILD_DIR

I believe we are in the early days of the project. The project is still experimental.

`std::fs::canonicalize` always converts paths to UNC on Windows.
However, a plenty of software lacks support for this naming convention,
notably Microsoft's C/C++ Compiler.

`dunce::canonicalize` is a commonly used solution that picks the most
compatible path representation on Windows and calls `fs::canonicalize`
on any other platform.
Previously, we assumed that the source directory is a parent of
`NGX_OBJS`. This isn't true, `--builddir` can point to an absolute path,
and source directory can be read-only. Specifying an explicit source
path and resolving includes agains it fixes the build for such
configurations.
The `NGX_OBJS` name was taken from the nginx buildsystem, but it is an
internal detail there, so the name wasn't really meaningful.  Now that
we have `NGINX_SOURCE_DIR`, there's a benefit in more consistent naming.

Technically, not a breaking change, as we haven't had a release with the
old variable name.
@bavshin-f5 bavshin-f5 force-pushed the portability branch 3 times, most recently from e6426f9 to f8bff78 Compare February 3, 2025 22:35
@bavshin-f5 bavshin-f5 requested a review from xeioex February 3, 2025 22:49
if [ "$NGX_DEBUG" = YES ]; then
ngx_cargo_default_profile=ngx-debug
else
ngx_cargo_default_profile=ngx-release
Copy link
Contributor

Choose a reason for hiding this comment

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

When building without --with-debug I get

...
objs/ngx_modules.o \                                                                                                                              
-L/media/psf/workspace/nginx/nginScript/quickjs-ng/build -lpthread -lcrypt objs/ngx_rust_examples/ngx-release/examples/libawssig.a -lm objs/ngx_ru
st_examples/ngx-release/examples/libcurl.a objs/ngx_rust_examples/ngx-release/examples/libupstream.a objs/ngx_rust_examples/ngx-release/examples/l
ibhttporigdst.a -lpcre2-8 -lssl -lcrypto -lpthread -lz \                                                                                          
-Wl,-E                                                                                                                                            
/usr/bin/ld: objs/ngx_rust_examples/ngx-release/examples/libcurl.a(curl-de3e6fe882b906f0.core-f5a882967048065e.core.ecb3f420ec5f4bcf-cgu.0.rcgu.o.
rcgu.o): in function `core::num::from_str_radix_panic_rt':                                                                                        
/rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/core/src/num/mod.rs:1399: multiple definition of `core::num::from_str_radix_panic_rt'; obj
s/ngx_rust_examples/ngx-release/examples/libawssig.a(awssig-22b3aae1e52dce40.core-f5a882967048065e.core.ecb3f420ec5f4bcf-cgu.0.rcgu.o.rcgu.o):/rus
tc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/core/src/num/mod.rs:1399: first defined here                                                  
/usr/bin/ld: objs/ngx_rust_examples/ngx-release/examples/libcurl.a(curl-de3e6fe882b906f0.core-f5a882967048065e.core.ecb3f420ec5f4bcf-cgu.0.rcgu.o.
rcgu.o): in function `...                                                                                                                         
u.0.rcgu.o.rcgu.o): in function `core::fmt::num::<impl core::fmt::LowerHex for i8>::fmt':                                                         
/usr/bin/ld: objs/ngx_rust_examples/ngx-release/examples/libcurl.a(curl-de3e6fe882b906f0.core-f5a882967048065e.core.ecb3f420ec5f4bcf-cgu.0.rcgu.o.
rcgu.o): in function `core::fmt::num::<impl core::fmt::UpperHex for i8>::fmt':                                                                    
/rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/core/src/fmt/num.rs:155: multiple definition of `core::fmt::num::<impl core::fmt::UpperHex
 for u8>::fmt'; objs/ngx_rust_examples/ngx-release/examples/libawssig.a(awssig-22b3aae1e52dce40.core-f5a882967048065e.core.ecb3f420ec5f4bcf-cgu.0.
rcgu.o.rcgu.o):/rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/core/src/fmt/num.rs:155: first defined here                                
/usr/bin/ld: objs/ngx_rust_examples/ngx-release/examples/libcurl.a(curl-de3e6fe882b906f0.core-f5a882967048065e.core.ecb3f420ec5f4bcf-cgu.0.rcgu.o.
rcgu.o): in function `core::fmt::num::imp::<impl core::fmt::Display for u64>::fmt':                                                               
/rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/core/src/fmt/num.rs:313: multiple definition of `core::fmt::num::imp::<impl core::fmt::Dis
play for usize>::fmt'; objs/ngx_rust_examples/ngx-release/examples/libawssig.a(awssig-22b3aae1e52dce40.core-f5a882967048065e.core.ecb3f420ec5f4bcf
-cgu.0.rcgu.o.rcgu.o):/rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/core/src/fmt/num.rs:313: first defined here                         
/usr/bin/ld: objs/ngx_rust_examples/ngx-release/examples/libcurl.a(curl-de3e6fe882b906f0.core-f5a882967048065e.core.ecb3f420ec5f4bcf-cgu.0.rcgu.o.
rcgu.o): in function `core::fmt::Formatter::write_str':                                                                                           
/rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/core/src/fmt/mod.rs:1625: multiple definition of `<core::fmt::Formatter as core::fmt::Writ
e>::write_str'; objs/ngx_rust_examples/ngx-release/examples/libawssig.a(awssig-22b3aae1e52dce40.core-f5a882967048065e.core.ecb3f420ec5f4bcf-cgu.0.
rcgu.o.rcgu.o):/rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/core/src/fmt/mod.rs:1625: first defined here                               
/usr/bin/ld: objs/ngx_rust_examples/ngx-release/examples/libcurl.a(curl-de3e6fe882b906f0.core-f5a882967048065e.core.ecb3f420ec5f4bcf-cgu.0.rcgu.o.
rcgu.o): in function `core::fmt::num::<impl core::fmt::LowerHex for i64>::fmt':                                                                   
/rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/core/src/fmt/num.rs:155: multiple definition of `core::fmt::num::<impl core::fmt::LowerHex
 for usize>::fmt'; objs/ngx_rust_examples/ngx-release/examples/libawssig.a(awssig-22b3aae1e52dce40.core-f5a882967048065e.core.ecb3f420ec5f4bcf-cgu
.0.rcgu.o.rcgu.o):/rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/core/src/fmt/num.rs:155: first defined here                             
/usr/bin/ld: objs/ngx_rust_examples/ngx-release/examples/libcurl.a(curl-de3e6fe882b906f0.core-f5a882967048065e.core.ecb3f420ec5f4bcf-cgu.0.rcgu.o.
rcgu.o): in function `core::fmt::num::<impl core::fmt::UpperHex for i64>::fmt':                                                                   
/rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/core/src/fmt/num.rs:155: multiple definition of `core::fmt::num::<impl core::fmt::UpperHex
 for usize>::fmt'; objs/ngx_rust_examples/ngx-release/examples/libawssig.a(awssig-22b3aae1e52dce40.core-f5a882967048065e.core.ecb3f420ec5f4bcf-cgu
.0.rcgu.o.rcgu.o):/rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/core/src/fmt/num.rs:155: first defined here                             
/usr/bin/ld: objs/ngx_rust_examples/ngx-release/examples/libcurl.a(curl-de3e6fe882b906f0.core-f5a882967048065e.core.ecb3f420ec5f4bcf-cgu.0.rcgu.o.
rcgu.o): in function `core::fmt::num::<impl core::fmt::LowerHex for i64>::fmt':                                                                   
/rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/core/src/fmt/num.rs:155: multiple definition of `core::fmt::num::<impl core::fmt::LowerHex
 for u64>::fmt'; objs/ngx_rust_examples/ngx-release/examples/libawssig.a(awssig-22b3aae1e52dce40.core-f5a882967048065e.core.ecb3f420ec5f4bcf-cgu.0
.rcgu.o.rcgu.o):/rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/core/src/fmt/num.rs:155: first defined here                               
/usr/bin/ld: objs/ngx_rust_examples/ngx-release/examples/libcurl.a(curl-de3e6fe882b906f0.core-f5a882967048065e.core.ecb3f420ec5f4bcf-cgu.0.rcgu.o.
rcgu.o): in function `core::fmt::num::<impl core::fmt::UpperHex for i64>::fmt':                                                                   
/rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/core/src/fmt/num.rs:155: multiple definition of `core::fmt::num::<impl core::fmt::UpperHex
 for u64>::fmt'; objs/ngx_rust_examples/ngx-release/examples/libawssig.a(awssig-22b3aae1e52dce40.core-f5a882967048065e.core.ecb3f420ec5f4bcf-cgu.0
.rcgu.o.rcgu.o):/rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/core/src/fmt/num.rs:155: first defined here                               
collect2: error: ld returned 1 exit status                                                                                                        
make[1]: *** [objs/Makefile:310: objs/nginx] Error 1                                                                                              
make[1]: Leaving directory '/media/psf/workspace/nginx/nginx'                                                                                     
make: *** [Makefile:10: build] Error 2                                                                                                            
(747 of 929): multiple definition of `core::num::from_str_radix_panic_rt'; objs/ngx_rust_examples/ngx-release/examples/libawssig.a(awssig-22b3aae1
e52dce40.core-f5a882967048065e.core.ecb3f420ec5f4bcf-cgu.0.rcgu.o.rcgu.o):/rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/core/src/num/mod
.rs:1399: first defined here   

Copy link
Contributor

Choose a reason for hiding this comment

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

Removing lto = "thin" from profile.ngx-release solves the issue.

examples/auto/rust Show resolved Hide resolved
examples/auto/rust Outdated Show resolved Hide resolved
examples/auto/rust Outdated Show resolved Hide resolved
Copy link
Contributor

@xeioex xeioex left a comment

Choose a reason for hiding this comment

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

Otherwise looks good.

examples/auto/rust Show resolved Hide resolved
A new file, `examples/auto/rust`, can be used in the module projects
to simplify the build and hide most of the platform-specific details.

 `examples/config` and `examples/config.make` are reimplemented using
the library.
@bavshin-f5
Copy link
Member Author

There are remaining problems with the approach here:

  • LTO is not supported
  • as we delegate the linking step to the nginx buildsystem, it lacks some useful linker flags.

Combined, this results in a much larger binary and missed optimization opportunities. We could attempt to address these issues by using Rust cdylib target for the shared modules and synthesizing a single staticlib crate for all the static modules. (See also: https://internals.rust-lang.org/t/rust-staticlibs-and-optimizing-for-size/5746)

It's also possible to simplify the target path logic with cargo rustc ... -- --emit=link=objs/ngx_http_example_module.a, passing an explicit (absolute!) path to the build artifacts (there's also -o, but it produces hashed libraries when used with cargo rustc: libawssig-8a15962bafeb450c.a).

But all of the above is not essential and could be fixed later, so I'm going to stop chasing the perfect solution and merge this as is.

@bavshin-f5 bavshin-f5 merged commit 9d0cee9 into nginx:master Feb 8, 2025
10 checks passed
@bavshin-f5 bavshin-f5 deleted the portability branch February 8, 2025 00:24
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