lighttpd build.sh simplification#100
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @gstrauss, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request simplifies the build.sh script for lighttpd in the phoenix-rtos-ports repository. It aims to reduce technical debt by leveraging the build.subr script for common build tasks such as downloading, unpacking, and patching. The changes also include adjustments to the configuration process, using the lighttpd.conf file to determine the modules to include in the static build.
Highlights
- build.subr usage: The build script now uses
build.subrfunctions for downloading, unpacking, and applying patches, streamlining the build process. - Configuration Simplification: The configuration step is simplified by using the
lighttpd.conffile to determine the modules to include in the static build, and the script now usesgrepto extract the modules from the config file. - Dependency Management: The script now uses PKG_SOURCE and PKG_SOURCE_URL variables for specifying the package source, and removes the need for separate variables for the package name and version.
Changelog
- lighttpd/build.sh
- Replaced manual download, unpack, and patch application with
build.subrfunctions. - Simplified variable definitions for package source and version.
- Modified the configuration process to use
lighttpd.conffor module selection. - Removed the creation of marker files for applied patches.
- Adjusted the configure command to align with the new build process.
- The grep command now filters out commented lines in lighttpd.conf
- Replaced manual download, unpack, and patch application with
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
A script's refined,
Technical debt now declined,
Build process flows,
As simplicity grows,
A cleaner port defined.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
The pull request aims to simplify the build.sh script for lighttpd by leveraging build.subr. This is a good step towards reducing technical debt and improving maintainability. However, there are several points that need to be addressed before merging, particularly regarding the configuration options and outdated dependencies.
Summary of Findings
- Outdated dependencies: The pull request description mentions that openssl 1.1.1a and PCRE in phoenix-rtos-ports are outdated and unsupported. openssl 1.1.1a is a security risk and should be updated. PCRE should be updated to PCRE2.
- Configuration options: The pull request description raises questions about the purpose of
CFLAGS="${CFLAGS}" LDFLAGS="${LDFLAGS}" AR_FLAGS="-r",--disable-ipv6, and--disable-mmapin theconfigureline. These should be clarified. - Static build configuration: The pull request description suggests a better way to configure lighttpd static build rather than grep'ing modules from lighttpd.conf. A separate .patch file in phoenix-rtos-ports with a sample set of modules is proposed.
Merge Readiness
The pull request is not ready to be merged due to the presence of outdated dependencies (openssl 1.1.1a and PCRE), unclear configuration options, and the need for a better approach to configure the lighttpd static build. These issues need to be addressed before merging. I am unable to approve this pull request, and recommend that others review and approve this code before merging.
|
( |
|
That gemini-code-assist signal to noise ratio is low. Besides misinterpreting the initial post, it produces a whole lot of redundant noise. I do not find it helpful, and I am not sure the pages of noise are useful to Phoenix RTOS developers who already see to have trouble task-switching to external-contributor PRs to update them more than once a month. |
|
I understand your concerns about the signal-to-noise ratio of my previous review. I'll strive to be more concise and targeted in my feedback going forward. I will focus on providing actionable suggestions and avoid stating the obvious. Thanks for the feedback. |
* build.sh wrap configure into multiple lines * remove LIGHTTPD_CFLAGS="-DPHOENIX" -DPHOENIX no longer used; prior patches deleted
ignore commented-out modules when grep'ing modules from lighttpd.conf in phoenix-rtos-project sample project
* use b_port_download b_port_apply_patches * download lighttpd-$ver.tar.xz instead of lighttpd-$ver.tar.gz
gstrauss
force-pushed
the
lighttpd-build.sh
branch
from
8b7ff0b to
af4e5db
Compare
|
@agkaminski, @Darchiv ping |
1 participant
Description
lighttpd build.sh simplification
Follow-up to (#90 (comment)) to use build.subr
Motivation and Context
Simplify. Reduce technical debt.
Types of changes
How Has This Been Tested?
Checklist:
Special treatment
Questions
CFLAGS="${CFLAGS}" LDFLAGS="${LDFLAGS}" AR_FLAGS="-r"added to theconfigureline?--disable-ipv6?--disable-mmap?Shipping openssl 1.1.1a in phoenix-rtos-ports is a * SECURITY BUG *. Please update.
https://www.pcre.org/