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

Wasm-wc: Add nxt_unit.o as a dependency in the auto script #1148

Merged
merged 2 commits into from
Feb 22, 2024

Conversation

ac000
Copy link
Member

@ac000 ac000 commented Feb 22, 2024

Rather than calling make itself to build nxt_unit.o make nxt_unit.o a dependency of the main module build target.

After this patch

$ make
cc -c -pipe -fPIC -fvisibility=hidden -O0 -W -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wmissing-prototypes  -g   -I src -I build/include   \
                      \
                     \
-o build/src/nxt_unit.o \
-MMD -MF build/src/nxt_unit.dep -MT build/src/nxt_unit.o \
src/nxt_unit.c

Compared to before

$ make
make build/src/nxt_unit.o
make[1]: Entering directory '/home/andrew/src/unit'
cc -c -pipe -fPIC -fvisibility=hidden -O0 -W -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wmissing-prototypes  -g   -I src -I build/include   \
                      \
                     \
-o build/src/nxt_unit.o \
-MMD -MF build/src/nxt_unit.dep -MT build/src/nxt_unit.o \
src/nxt_unit.c

@ac000 ac000 requested a review from thresheek February 22, 2024 01:37
@thresheek
Copy link
Member

The second patch also looks good to me. Works as intended.

Thanks!

Rather than calling make itself to build nxt_unit.o make nxt_unit.o a
dependency of the main module build target.

Reported-by: Konstantin Pavlov <thresh@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
cargo build creates the language module under
src/wasm-wasi-component/target/release/libwasm_wasi_component.so and not
build/lib/unit/modules/wasm_wasi_component.unit.so which is what we were
using as a target dependency in the Makefile which doesn't exist so this
resulted in the following

  $ make wasm-wasi-component-install
  cargo build --release --manifest-path src/wasm-wasi-component/Cargo.toml
      Finished release [optimized] target(s) in 0.17s
  install -d /opt/unit/modules
  install -p src/wasm-wasi-component/target/release/libwasm_wasi_component.so \
          /opt/unit/modules/wasm_wasi_component.unit.so

I.e it wanted to rebuild the module, after this patch we get the more
correct

  $ make wasm-wasi-component-install
  install -d /opt/unit/modules
  install -p src/wasm-wasi-component/target/release/libwasm_wasi_component.so \
          /opt/unit/modules/wasm_wasi_component.unit.so

This is all a little ugly because we're fighting against cargo wanting
to do its own thing and this wasm-wasi-component language module build
process is likely going to get some re-working anyway, so this will do
for now.

Reported-by: Konstantin Pavlov <thresh@nginx.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
@ac000
Copy link
Member Author

ac000 commented Feb 22, 2024

Rebased with master

$ git range-diff c5956b79...d54af163
-:  -------- > 1:  3f805bc6 Packages: added wasm-wasi-component module packaging for deb-based distros
-:  -------- > 2:  7a640556 Packages: added wasm-wasi-component module packaging for rpm-based distros
1:  c0ee1461 = 3:  7b13c306 Wasm-wc: Add nxt_unit.o as a dependency in the auto script
2:  c5956b79 = 4:  d54af163 Wasm-wc: Use the cargo build output as the make target dependency

@ac000 ac000 merged commit d54af16 into nginx:master Feb 22, 2024
15 checks passed
@ac000 ac000 deleted the wasm-wasi-make-fix branch February 22, 2024 03:22
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