Skip to content

Conversation

simayosi
Copy link

Proposed changes

In Makefile.module-*, variable names should use underscores (_) instead of dashes (-) in module names.
This convention is followed in code such as alpine/Makefile:114 and debian/Makefile:116.
For example, the version variable for the headers-more module in Makefile.headers-more is defined asMODULE_VERSION_headers_more.

However, build_module.sh currently generates Makefile.module-$MODULE_NAME using the module name as-is in variable names, without converting dashes to underscores.
As a result, modules with dashes in their names cannot be built correctly.

This patch addresses the issue by ensuring that module names used in variable names have dashes replaced with underscores.

Checklist

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

@thresheek
Copy link
Member

Hello @simayosi !

Can you please describe the issue with build_module.sh you're trying to fix?

Since I've just tried ./build_module.sh -n headers-more https://github.com/openresty/headers-more-nginx-module/archive/refs/tags/v0.38.tar.gz and on my Ubuntu machine it happily built the resulting packages with the dashes removed:

$ ls -1 build-module-artifacts/
nginx-module-headersmore-dbg_1.27.5+1.0-1~noble_arm64.deb
nginx-module-headersmore_1.27.5+1.0-1~noble_arm64.deb

Which is probably less than optimal, but is already used in leaf projects like docker-nginx, where we expect the dashes to be removed too: https://github.com/nginx/docker-nginx/blob/6b055c471a6619d4c81671e682a6d6affe0cf9c0/modules/Dockerfile#L56

Thank you!

@simayosi
Copy link
Author

simayosi commented May 5, 2025

Hi @thresheek,

Thanks for your comment and for testing on Ubuntu.
Apologies, I should have included the environment where I hit the problem.
The issue I encountered happens specifically on Alpine Linux.

Your test prompted me to re-examine the build_module.sh script more closely to understand why it failed on Alpine.
After that re-check, I found that the actual source of the problem on Alpine lies in line 238 of the script.
The problem is caused by a difference in the tr implementation on Alpine Linux (BusyBox tr) compared to GNU tr.

First, both GNU tr and POSIX tr treat 'm-n' (without within []) as the characters from m through n.
Next, although both the GNU tr manpage and POSIX tr specification do not specify the behavior for unrecognized backslash escape sequences, GNU tr treats \- and \. as - and . respectively, while Alpine's tr treats \-\. as characters from \ to \, i.e., \ only, and ..

Consequently, to ensure this module name cleaning step works consistently across different systems, the line should be the following:

MODULE_NAME_CLEAN=`echo $MODULE_NAME | tr 'A-Z' 'a-z' | tr '/_.\t -' '[\n*]' | grep -ve nginx | tr -d '\n'`

However, in all of the module Makefiles included in this repository for modules with - in their name, variable names are defined with replacing dashes with underscores in their module names.
Therefore, I propose that build_module.sh leaves dashes in MODULE_NAME_CLEAN as is as the following and converts dashes to underscores in creating module Makefile by applying my patch.

MODULE_NAME_CLEAN=`echo $MODULE_NAME | tr 'A-Z' 'a-z' | tr '/_.\t ' '[\n*]' | grep -ve nginx | tr -d '\n'`

Thank you!

@simayosi
Copy link
Author

I've added a follow-up commit to fix MODULE_NAME_CLEAN. This commit, along with my previous one, addresses the issue based on my proposal.
I tested this fix using Dockerfiles for Alpine and Debian, based on the official Nginx images.

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