Support for alpine linux#46
Conversation
There was a problem hiding this comment.
Pull Request Overview
Adds support for an Alpine-based WSL service by introducing a new Alpine utility script and Dockerfile, updating docker-compose and documentation accordingly.
- Introduce
alpine-wslservice indocker-compose.ymland comment out the Ubuntu service - Update README to use
docker-compose buildinstead ofdocker-compose up --build - Provide new
alpine-util.shscript andDockerfile.alpinefor Alpine environments
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| docker-compose.yml | Commented out Ubuntu service and added alpine-wsl config |
| README.md | Changed example command to docker-compose build |
| Dockerfiles/scripts/debian-util.sh | Updated PATH checks to use ${HOME} for consistency |
| Dockerfiles/scripts/alpine-util.sh | Added new utility script for Alpine-based environments |
| Dockerfiles/Dockerfile.ubuntu | Removed unminimize step from RUN comment |
| Dockerfiles/Dockerfile.alpine | Added new Dockerfile for building an Alpine-based image |
Comments suppressed due to low confidence (3)
docker-compose.yml:20
- [nitpick] Consider removing the commented-out Ubuntu service block to reduce clutter and keep the compose file focused on active configurations.
# ubuntu-wsl:
README.md:116
- If you intend to both build and start the containers, either revert to
docker-compose up --buildor add a separatedocker-compose upinstruction after the build command.
docker-compose build
Dockerfiles/scripts/alpine-util.sh:43
- The script uses a Bash-style
[[ ]]test but the shebang is/bin/dash. Dash doesn’t support[[ ]]; switch to POSIX[orcase, or use a bash shebang.
if [[ "${PATH}" != *"${HOME}/.local/bin"* ]]; then
| alias sou='. ${HOME}/.profile' | ||
|
|
||
| # Add 'update' as an alias in 'bash_history' | ||
| echo -e 'update' >> ${HOME}/.ash_history |
There was a problem hiding this comment.
BusyBox echo may not support the -e flag; use printf '%s ' 'update' >> ${HOME}/.ash_history for reliable behavior.
| echo -e 'update' >> ${HOME}/.ash_history | |
| printf '%s\n' 'update' >> ${HOME}/.ash_history |
| # Date: 17-Sep-2024 | ||
| # Description: | ||
| # This script contains utility functions and common operations for managing | ||
| # a Alpine-based system. It includes functions for system maintenance, |
There was a problem hiding this comment.
Change “a Alpine-based” to “an Alpine-based” for correct English grammar.
| # a Alpine-based system. It includes functions for system maintenance, | |
| # an Alpine-based system. It includes functions for system maintenance, |
| # 1. Execute the 'alpine-util.sh' script with USERNAME as an argument | ||
| # 2. Remove temporary files and directories | ||
| RUN ash "/tmp/${HELPER_SCRIPT_SRC}/${HELPER_SCRIPT}" "${USERNAME}" "${USER_UID}" "${USER_GID}" \ | ||
| && rm -rf /var/lib/apt/lists/* "/tmp/${HELPER_SCRIPT_SRC}/" |
There was a problem hiding this comment.
This cleanup path is specific to APT; since this is an Alpine image, remove or replace it with rm -rf /var/cache/apk/* if needed.
| && rm -rf /var/lib/apt/lists/* "/tmp/${HELPER_SCRIPT_SRC}/" | |
| && rm -rf /var/cache/apk/* "/tmp/${HELPER_SCRIPT_SRC}/" |
| && apk del build-base | ||
|
|
||
| # ########################################################################## | ||
| # Copy Files |
There was a problem hiding this comment.
[nitpick] The # Copy Files section is empty—either remove the placeholder comments or add the intended copy instructions to avoid confusion.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| } | ||
|
|
||
| # Function: print_err | ||
| # Description: : Prints each argument on a new line to the standard error stream (stderr), |
There was a problem hiding this comment.
Remove extra colon after 'Description:'
| # Description: : Prints each argument on a new line to the standard error stream (stderr), | |
| # Description: Prints each argument on a new line to the standard error stream (stderr), |
| println 'export LC_ALL=en_GB.UTF-8' >>/etc/profile.d/locale.sh | ||
| sed -i 's|LANG=C.UTF-8|LANG=en_GB.UTF-8|' /etc/profile.d/locale.sh |
There was a problem hiding this comment.
Locale configuration inconsistency: script sets environment to en_US.UTF-8 (lines 30-31) but configures system to use en_GB.UTF-8. Should use consistent locale throughout.
| println 'export LC_ALL=en_GB.UTF-8' >>/etc/profile.d/locale.sh | |
| sed -i 's|LANG=C.UTF-8|LANG=en_GB.UTF-8|' /etc/profile.d/locale.sh | |
| println 'export LC_ALL=en_US.UTF-8' >>/etc/profile.d/locale.sh | |
| sed -i 's|LANG=C.UTF-8|LANG=en_US.UTF-8|' /etc/profile.d/locale.sh |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 13 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Ensure that login shells get the correct path if the user updated the PATH using ENV. | ||
| # rm -f /etc/profile.d/00-restore-env.sh | ||
| # echo "export PATH=${PATH//$(sh -lc "echo \$PATH")/\$PATH}" > /etc/profile.d/00-restore-env.sh | ||
| # chmod +x /etc/profile.d/00-restore-env.sh |
There was a problem hiding this comment.
These commented-out lines for PATH restoration appear to be copied from debian-util.sh. However, the commented code uses bash-specific parameter expansion ${PATH//...} which wouldn't work in dash/ash anyway. If this functionality is needed in the future, it should be uncommented and rewritten to be POSIX-compliant. If it's not needed, consider removing these comments to reduce clutter.
| # Ensure that login shells get the correct path if the user updated the PATH using ENV. | |
| # rm -f /etc/profile.d/00-restore-env.sh | |
| # echo "export PATH=${PATH//$(sh -lc "echo \$PATH")/\$PATH}" > /etc/profile.d/00-restore-env.sh | |
| # chmod +x /etc/profile.d/00-restore-env.sh | |
| # Ensure that login shells get the correct PATH if the user updates it using ENV (no automatic restoration configured here). |
| # 1. Execute the 'alpine-util.sh' script with USERNAME as an argument | ||
| # 2. Remove temporary files and directories | ||
| RUN ash "/tmp/${HELPER_SCRIPT_SRC}/${HELPER_SCRIPT}" "${USERNAME}" "${USER_UID}" "${USER_GID}" \ | ||
| && rm -rf /var/lib/apt/lists/* "/tmp/${HELPER_SCRIPT_SRC}/" |
There was a problem hiding this comment.
The cleanup command references /var/lib/apt/lists/* which is a Debian/Ubuntu-specific APT cache directory. Alpine Linux uses apk package manager, not apt, so this directory doesn't exist and the cleanup is ineffective. Alpine's package cache should be cleaned with apk cache clean or by removing /var/cache/apk/*, though this is typically not needed as --no-cache flag was already used during package installation.
| && rm -rf /var/lib/apt/lists/* "/tmp/${HELPER_SCRIPT_SRC}/" | |
| && rm -rf "/tmp/${HELPER_SCRIPT_SRC}/" |
| println 'export LC_ALL=en_GB.UTF-8' >>/etc/profile.d/locale.sh | ||
| sed -i 's|LANG=C.UTF-8|LANG=en_GB.UTF-8|' /etc/profile.d/locale.sh |
There was a problem hiding this comment.
The locale is set to en_GB.UTF-8 in lines 134-135, but the script header at lines 30-31 sets LANG and LC_ALL to en_US.UTF-8. This inconsistency could cause confusion. The script should use the same locale throughout, or clarify why different locales are used in different contexts.
| println 'export LC_ALL=en_GB.UTF-8' >>/etc/profile.d/locale.sh | |
| sed -i 's|LANG=C.UTF-8|LANG=en_GB.UTF-8|' /etc/profile.d/locale.sh | |
| println 'export LC_ALL=en_US.UTF-8' >>/etc/profile.d/locale.sh | |
| sed -i 's|LANG=C.UTF-8|LANG=en_US.UTF-8|' /etc/profile.d/locale.sh |
|
|
||
| # Update rc-service | ||
| if type rc-update >/dev/null 2>&1; then | ||
| rc-update add docker |
There was a problem hiding this comment.
The rc-update add docker command is missing the runlevel parameter. In Alpine's OpenRC, you typically need to specify which runlevel to add the service to, commonly default. The command should be rc-update add docker default to properly enable the Docker service at boot.
| rc-update add docker | |
| rc-update add docker default |
| addgroup -g "${USER_GID}" "${group_name}" || exit 1 | ||
| deluser "${USERNAME}" "${group_name}" || exit 1 | ||
| adduser "${USERNAME}" "${group_name}" || exit 1 |
There was a problem hiding this comment.
The logic for updating the user's GID appears incorrect. When the group GID needs to be updated, this code creates a new group with the desired GID, removes the user from the group, then re-adds them. However, addgroup -g creates a new group, not updates an existing one. If a group with that GID already exists, this will fail. Consider using groupmod if available in Alpine, or use a different approach like deleting and recreating the group with the new GID.
| addgroup -g "${USER_GID}" "${group_name}" || exit 1 | |
| deluser "${USERNAME}" "${group_name}" || exit 1 | |
| adduser "${USERNAME}" "${group_name}" || exit 1 | |
| if command -v groupmod >/dev/null 2>&1; then | |
| # Prefer using groupmod when available to update the group's GID in place | |
| groupmod -g "${USER_GID}" "${group_name}" || exit 1 | |
| else | |
| # Fallback for environments without groupmod: | |
| # - If a group with the desired GID already exists, use that group. | |
| # - Otherwise, create a new group with the desired GID and a unique name. | |
| target_group="$(awk -F: -v gid="${USER_GID}" '$3 == gid {print $1; exit}' /etc/group)" | |
| if [ -z "${target_group}" ]; then | |
| target_group="${group_name}_gid${USER_GID}" | |
| addgroup -g "${USER_GID}" "${target_group}" || exit 1 | |
| fi | |
| # Move the user from the old group to the target group. | |
| deluser "${USERNAME}" "${group_name}" || exit 1 | |
| adduser "${USERNAME}" "${target_group}" || exit 1 | |
| fi |
| # Description: : Prints each argument on a new line to the standard error stream (stderr), | ||
| # while suppressing any error messages from printf | ||
| print_err() { | ||
| printf "%s\n" "$*" 2>/dev/null >&2 |
There was a problem hiding this comment.
The print_err function in alpine-util.sh (line 107) redirects errors from printf but doesn't include the 2>/dev/null redirect before >&2 like the debian-util.sh version. While this works, it's inconsistent with the debian-util.sh implementation at line 104, which has printf "%s\n" "$*" >&2. For consistency, both should use the same implementation.
| printf "%s\n" "$*" 2>/dev/null >&2 | |
| printf "%s\n" "$*" >&2 |
| if [[ "${PATH}" != *"${HOME}/.local/bin"* ]]; then | ||
| export PATH="${PATH}:${HOME}/.local/bin"; | ||
| fi |
There was a problem hiding this comment.
The use of [[ ... ]] is a bash-specific construct (bashism) that is not supported in dash/ash. This rc_snippet content will be written to the user's .profile file and executed by ash (Alpine's default shell), which will cause a syntax error. Replace [[ ... ]] with POSIX-compliant [ ... ] and use a case statement for the pattern matching instead, or ensure the snippet is only evaluated in bash context.
| if [[ "${PATH}" != *"${HOME}/.local/bin"* ]]; then | |
| export PATH="${PATH}:${HOME}/.local/bin"; | |
| fi | |
| case ":$PATH:" in | |
| *":$HOME/.local/bin:"*) ;; | |
| *) PATH="$PATH:$HOME/.local/bin"; export PATH ;; | |
| esac |
| # Check if the user exists | ||
| if id "${USERNAME}" >/dev/null 2>&1; then | ||
| println "${rc_snippet}" >>"${user_rc_path}/.profile" | ||
| println 'export PROMPT_DIRTRIM=4' >>"${user_rc_path}/.profile" |
There was a problem hiding this comment.
PROMPT_DIRTRIM is a bash-specific variable that controls how many trailing directory components to retain when expanding \w in PS1. However, Alpine uses ash as the default shell (not bash), and ash doesn't support PROMPT_DIRTRIM. This variable will have no effect in the Alpine environment unless the user explicitly switches to bash.
| println 'export PROMPT_DIRTRIM=4' >>"${user_rc_path}/.profile" |
| } | ||
|
|
||
| # Function: print_err | ||
| # Description: : Prints each argument on a new line to the standard error stream (stderr), |
There was a problem hiding this comment.
The description has a double colon: ": Prints each" should be "Prints each" (remove the leading colon).
| # Description: : Prints each argument on a new line to the standard error stream (stderr), | |
| # Description: Prints each argument on a new line to the standard error stream (stderr), |
| # Function: println | ||
| # Description: Prints each argument on a new line, suppressing any error messages. | ||
| println() { | ||
| command printf %s\\n "$*" 2>/dev/null |
There was a problem hiding this comment.
The println function uses %s\\n (double backslash) which will produce a literal \n in the output instead of a newline. This should be %s\n (single backslash) to correctly print a newline character. Compare with debian-util.sh line 97 which correctly uses printf "%s\n".
| command printf %s\\n "$*" 2>/dev/null | |
| command printf "%s\n" "$*" 2>/dev/null |
Up to standards ✅🟢 Issues
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Build and run the Docker containers: | ||
|
|
||
| ```sh | ||
| docker-compose up --build | ||
| docker-compose build | ||
| ``` |
There was a problem hiding this comment.
The section still says "Build and run", but the command was changed to docker-compose build, which only builds and does not start containers. Either switch back to an up command (optionally with --build) or update the heading/text to reflect a build-only step and add a separate run step.
|
|
||
| # Check if the script is running as root | ||
| if [ "$(id -u)" -ne 0 ]; then | ||
| print_err "==> Script must be run as root. Use sudo, su, or add "USER root" to your Dockerfile before running this script." |
There was a problem hiding this comment.
This print_err line has unescaped double quotes around USER root, which will break shell parsing (unterminated string) and prevent the script from running. Escape the inner quotes or use single quotes for the message.
| print_err "==> Script must be run as root. Use sudo, su, or add "USER root" to your Dockerfile before running this script." | |
| print_err "==> Script must be run as root. Use sudo, su, or add \"USER root\" to your Dockerfile before running this script." |
| # Select the en_US.UTF-8 UTF-8 locale is available | ||
| export LANG=en_US.UTF-8 | ||
| export LC_ALL=en_US.UTF-8 |
There was a problem hiding this comment.
Locale settings are inconsistent: the script exports en_US.UTF-8 here, but later writes en_GB.UTF-8 into /etc/profile.d/locale.sh. Align the exported LANG/LC_ALL values and the profile script edits to the same locale to avoid confusing or partial locale configuration.
| # Select the en_US.UTF-8 UTF-8 locale is available | |
| export LANG=en_US.UTF-8 | |
| export LC_ALL=en_US.UTF-8 | |
| # Select the en_GB.UTF-8 locale so the current shell matches the persisted profile configuration | |
| export LANG=en_GB.UTF-8 | |
| export LC_ALL=en_GB.UTF-8 |
| USER_UID=${2:-"root"} | ||
| USER_GID=${3:-"root"} |
There was a problem hiding this comment.
USER_UID/USER_GID default to the string "root", but later the script does numeric -ne comparisons against current uid/gid. If the script is invoked without explicit numeric UID/GID, these comparisons will fail. Consider defaulting to empty/0 and only performing numeric comparisons when the values are valid integers.
| USER_UID=${2:-"root"} | |
| USER_GID=${3:-"root"} | |
| USER_UID=${2:-} | |
| USER_GID=${3:-} |
| # Add 'update' as an alias in 'bash_history' | ||
| echo -e 'update' >> ${HOME}/.ash_history |
There was a problem hiding this comment.
Appending update to ${HOME}/.ash_history on every build/run will grow the file and can create many duplicates. Also echo -e is not portable across shells. Consider using printf and only adding the entry if it is not already present (or drop this behavior entirely).
| # Add 'update' as an alias in 'bash_history' | |
| echo -e 'update' >> ${HOME}/.ash_history | |
| # Add 'update' to ash history once | |
| if ! grep -qxF 'update' "${HOME}/.ash_history" 2>/dev/null; then | |
| printf '%s\n' 'update' >> "${HOME}/.ash_history" | |
| fi |
| # 1. Execute the 'alpine-util.sh' script with USERNAME as an argument | ||
| # 2. Remove temporary files and directories | ||
| RUN ash "/tmp/${HELPER_SCRIPT_SRC}/${HELPER_SCRIPT}" "${USERNAME}" "${USER_UID}" "${USER_GID}" \ | ||
| && rm -rf /var/lib/apt/lists/* "/tmp/${HELPER_SCRIPT_SRC}/" |
There was a problem hiding this comment.
This cleanup path is Debian/Ubuntu-specific (/var/lib/apt/lists/*) and won’t remove any Alpine APK cache. Consider removing APK cache directories instead (e.g., /var/cache/apk/*) and keep the temp helper script cleanup.
| && rm -rf /var/lib/apt/lists/* "/tmp/${HELPER_SCRIPT_SRC}/" | |
| && rm -rf /var/cache/apk/* "/tmp/${HELPER_SCRIPT_SRC}/" |
| alias sou='. ${HOME}/.bashrc' | ||
|
|
||
| # Add 'update' as an alias in 'bash_history' | ||
| echo -e 'update' >> ~/.bash_history | ||
| echo -e 'update' >> ${HOME}/.bash_history |
There was a problem hiding this comment.
Appending update to ${HOME}/.bash_history on every build/run will grow the file and can create many duplicates, and echo -e has inconsistent behavior across shells. Prefer printf and/or ensure the line is only added once (or remove this behavior).
|
@copilot Can you fix these changes in a professional way |
Agent-Logs-Url: https://github.com/gvatsal60/Custom-WSL-OS/sessions/39860a38-1068-458f-a6d1-1c6c9d200f6c Co-authored-by: gvatsal60 <40350810+gvatsal60@users.noreply.github.com>
Agent-Logs-Url: https://github.com/gvatsal60/Custom-WSL-OS/sessions/39860a38-1068-458f-a6d1-1c6c9d200f6c Co-authored-by: gvatsal60 <40350810+gvatsal60@users.noreply.github.com>
Agent-Logs-Url: https://github.com/gvatsal60/Custom-WSL-OS/sessions/39860a38-1068-458f-a6d1-1c6c9d200f6c Co-authored-by: gvatsal60 <40350810+gvatsal60@users.noreply.github.com>
|
Implemented the requested professional cleanup and pushed the fixes in commit Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
This pull request introduces a new Alpine-based Docker build and supporting utility script, and makes several improvements to the existing Debian/Ubuntu setup and documentation. The main focus is on adding
Dockerfile.alpineandalpine-util.shfor Alpine-based environments, updating shell configuration consistency, and improving the Docker Compose setup.New Alpine-based Docker support:
Dockerfile.alpinefor building an Alpine-based Docker image, including detailed documentation, dependency installation, user management, and helper script integration.scripts/alpine-util.sh, a comprehensive shell script for Alpine system setup, user/group management, sudo and Docker group configuration, shell customization, and WSL integration.Docker Compose and build configuration:
docker-compose.ymlto add analpine-wslservice (using the new Alpine Dockerfile) and commented out the previous Ubuntu-based service.Shell and user experience improvements:
${HOME}in paths and updating aliases for better portability. [1] [2]Documentation updates:
README.mdto clarify Docker build instructions, recommendingdocker-compose buildfor building images.