feat: modify cache & NGINX dir #72

JyJyJcr · 2024-04-02T02:13:23Z

Proposed changes

This PR add some environment variable options useful to develop modules using the SDK.

`CACHE_DIR`

Default: [nginx-sys's root]/.cache

Specify cache directory. With relative -> absolute path converting option provided in .cargo/config.toml like below, we are able to generate a project-owned cache dir:

[env]
CACHE_DIR = { value = ".cache", relative = true }

This option makes it easier to save and restore the cache, which speeds up CI/CD.

Sidenote

Originally the default location of the cache dir is in the parent dir of the manifest dir (= root dir) of nginx-sys: this is equivalent to set the default of CACHE_DIR as [nginx-sys's root]/../.cache. This setting resulted the cache dir to be generated in the root dir of ngx-rust as long as ngx-rust is treated as the main project (this is likely the originally intended behavior).
However, once nginx-sys is whether directly or indirectly used as a dependency of other projects, the repository was copied in $CARGO_HOME/registry/cache/index.crates.io-***/nginx-sys-***/ or $CARGO_HOME/git/checkouts/ngx-rust-***/***/, causing the cache dir to be generated in the same depth with the copies of crates. This was apparently undesirable.
So the default value of CACHE_DIR is changed to [nginx-sys's root]/.cache to prevent the "pollution" described above. Instead, ngx-rust also use CACHE_DIR to move the cache dir to the original location.

`NGINX_INSTALL_ROOT_DIR`

Default: {CACHE_DIR}/nginx

Specify the directory where the NGINX compiled in the series of build will be installed. By default, each of Nginx is installed in the sub-subdirectory labeled by the version, OS and Architecture. This option is useful at developing stage to try multiple versions with your own conf files (for the latter aim only, NGINX_INSTALL_DIR is more suited).

`NGINX_INSTALL_DIR`

Default: {NGINX_INSTALL_ROOT_DIR}/{NGX_VERSION}/{OS}-{Arch}

Specify the directory where the NGINX compiled in this build will be installed. This option is mainly intended to facilitate the test automation in CI/CD.

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

dekobon · 2024-04-02T20:10:01Z

Let's add this to the .gitignore file

nginx-sys/.cache/

dekobon

This is a solid PR! Thank you.

Could you please add the new .cache directory to the .gitignore file?
Also, please reply to the comment about using the cargo triple instead of the os-arch pattern. I do not have a strong opinion, so I would love to know what you think.

Once again, thank you for the contribution.

dekobon · 2024-04-02T20:17:31Z

README.md

@@ -38,9 +38,15 @@ cargo build --package=examples --examples --features=linux --release
 After compilation, the modules can be found in the path `target/release/examples/` ( with the `.so` file extension for
 Linux or `.dylib` for MacOS).

-Additionally, the folder  `.cache/nginx/{NGX_VERSION}/{OS}/` will contain the compiled version of NGINX used to build
+Additionally, the folder  `.cache/nginx/{NGX_VERSION}/{OS}-{Arch}/` will contain the compiled version of NGINX used to build


If we are going to encode architecture here, we may want to consider over to using the full cargo triple string because os and architecture are not enough information to tell you if you can actually run the binary on a system. For example, a library may be build for musl, and will not work on other glibc variants.

It looks like <arch><sub>-<vendor>-<sys>-<abi>.

What do you think of doing that instead?

Cargo triple strings contain enough information to replace {OS}-{Arch} since we currently only support linux and macos (if not, there are more variants listed here and here), so I agree to use the triple.
What I suspect is whether the triple is enough. Does Nginx binary vary depending on Linux distributions? For example, do we have to distinguish two same version Nginxs, one of which is compiled on Debian and the other is on CentOS?

Does Nginx binary vary depending on Linux distributions

It does only for the version created by the distros for their packaging systems. Many of those contain patches. Those binaries are considered "unsupported" / "unsupportable" from the perspective of NGINX. However, that doesn't really matter for ngx-rust, because we are building the binaries from scratch without any such patches, so I believe we do not need to encode any distro specific information. In short, we don't need to care about this for ngx-rust.

Let's change the PR to support the triple string, and I'll click the merge button.

JyJyJcr · 2024-04-03T06:18:16Z

Thank you for your rapid review.

Let's add this to the .gitignore file
nginx-sys/.cache/

We don't have to add this, because ngx-rust set CACHE_DIR to [ngx-rust's root]/.cache:
https://github.com/nginxinc/ngx-rust/blob/29076150102b5524df635ab114bca1160f8e7989/.cargo/config.toml#L15-L16

ngx-rust keeps generating cache in its root dir, same as before.

dekobon · 2024-04-03T16:09:20Z

Thank you for your rapid review.
Let's add this to the .gitignore file
nginx-sys/.cache/
We don't have to add this, because ngx-rust set CACHE_DIR to [ngx-rust's root]/.cache:

https://github.com/nginxinc/ngx-rust/blob/29076150102b5524df635ab114bca1160f8e7989/.cargo/config.toml#L15-L16

ngx-rust keeps generating cache in its root dir, same as before.

Oh, that's my oversight. Thank you for clarifying.

JyJyJcr · 2024-04-03T17:16:23Z

I changed the src dir name in the same manner to the Nginx dir:
https://github.com/nginxinc/ngx-rust/blob/0c930cabf7505e3f31cf021b74cfe810d60cdf5f/nginx-sys/build.rs#L275-L279
In addition, I added target-triple to dev-dependencies to fix log_test.rs to support new style of the directory structure:
https://github.com/nginxinc/ngx-rust/blob/0c930cabf7505e3f31cf021b74cfe810d60cdf5f/tests/log_test.rs#L18-L25

dekobon · 2024-04-03T18:35:23Z

README.md

@@ -38,9 +38,15 @@ cargo build --package=examples --examples --features=linux --release
 After compilation, the modules can be found in the path `target/release/examples/` ( with the `.so` file extension for
 Linux or `.dylib` for MacOS).

-Additionally, the folder  `.cache/nginx/{NGX_VERSION}/{OS}/` will contain the compiled version of NGINX used to build
+Additionally, the folder  `.cache/nginx/{NGX_VERSION}/{OS}-{Arch}/` will contain the compiled version of NGINX used to build


.cache/nginx/{NGX_VERSION}/{OS}-{Arch}/

One last thing, let's update the string in the readme to reflect the triple format.

dekobon · 2024-04-03T18:36:01Z

README.md

+
+* `CACHE_DIR` (default `[nginx-sys's root directory]/.cache`) - the directory containing cache files, means PGP keys, tarballs, PGP signatures, and unpacked source codes. It also contains compiled NGINX in default configuration.
+* `NGINX_INSTALL_ROOT_DIR` (default `{CACHE_DIR}/nginx`) - the directory containing the series of compiled NGINX in its subdirectories
+* `NGINX_INSTALL_DIR` (default `{NGINX_INSTALL_BASE_DIR}/{NGX_VERSION}/{OS}-{Arch}`) - the directory containing the NGINX compiled in the build


dekobon

Looks great. Let's get the readme updated and I will merge.

Once again, thank you for the stellar contribution!

JyJyJcr · 2024-04-03T18:50:28Z

Updated README.md. Thank you for the continuous support!

JyJyJcr added 6 commits April 2, 2024 03:36

feat: recognize CACHE_DIR

831ee3a

refactor: reduce duplicated call make_cache_dir()

8142309

feat: recognize NGX_INSTALL_ROOT_DIR

0c63b59

feat: recognize NGX_INSTALL_DIR

84f1713

docs: add info in README.md

12e8810

chore: fix clippy warning

2907615

dekobon requested changes Apr 2, 2024

View reviewed changes

!refactor: use target triple for nginx dir

0c930ca

dekobon reviewed Apr 3, 2024

View reviewed changes

dekobon approved these changes Apr 3, 2024

View reviewed changes

docs: update README.md

ecbce05

dekobon merged commit c4beb12 into nginx:master Apr 3, 2024
5 of 6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: modify cache & NGINX dir #72

feat: modify cache & NGINX dir #72

JyJyJcr commented Apr 2, 2024 •

edited

Loading

dekobon commented Apr 2, 2024

dekobon left a comment

dekobon Apr 2, 2024

JyJyJcr Apr 3, 2024 •

edited

Loading

dekobon Apr 3, 2024

JyJyJcr commented Apr 3, 2024 •

edited

Loading

dekobon commented Apr 3, 2024

JyJyJcr commented Apr 3, 2024 •

edited

Loading

dekobon Apr 3, 2024 •

edited

Loading

dekobon Apr 3, 2024

dekobon left a comment

JyJyJcr commented Apr 3, 2024

feat: modify cache & NGINX dir #72

feat: modify cache & NGINX dir #72

Conversation

JyJyJcr commented Apr 2, 2024 • edited Loading

Proposed changes

CACHE_DIR

Sidenote

NGINX_INSTALL_ROOT_DIR

NGINX_INSTALL_DIR

Checklist

dekobon commented Apr 2, 2024

dekobon left a comment

Choose a reason for hiding this comment

dekobon Apr 2, 2024

Choose a reason for hiding this comment

JyJyJcr Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

dekobon Apr 3, 2024

Choose a reason for hiding this comment

JyJyJcr commented Apr 3, 2024 • edited Loading

dekobon commented Apr 3, 2024

JyJyJcr commented Apr 3, 2024 • edited Loading

dekobon Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

dekobon Apr 3, 2024

Choose a reason for hiding this comment

dekobon left a comment

Choose a reason for hiding this comment

JyJyJcr commented Apr 3, 2024

JyJyJcr commented Apr 2, 2024 •

edited

Loading

`CACHE_DIR`

`NGINX_INSTALL_ROOT_DIR`

`NGINX_INSTALL_DIR`

JyJyJcr Apr 3, 2024 •

edited

Loading

JyJyJcr commented Apr 3, 2024 •

edited

Loading

JyJyJcr commented Apr 3, 2024 •

edited

Loading

dekobon Apr 3, 2024 •

edited

Loading