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

{bp-2902} interpreters/luamodules/luv: use "depends on" instead of "select" #2907

Merged
merged 2 commits into from
Dec 20, 2024

Conversation

jerpelea
Copy link
Contributor

Summary

interpreters/luamodules/luv: use "depends on" instead of "select"

Included
#2898

Impact

RELEASE

Testing

CI

@nuttxpr
Copy link

nuttxpr commented Dec 19, 2024

[Experimental Bot, please feedback here]

Fill In The Commit Message: This PR contains a Commit with an Empty Commit Message. Please fill in the Commit Message with the PR Summary.

No, this PR does not fully meet the NuttX requirements. While it provides a summary and links a related NuttX Apps PR, it lacks crucial information.

Here's what's missing:

  • Detailed Summary: The summary needs to explain why "select" should be replaced with "depends on." What problem does this solve? What are the benefits? What part of the code is affected (e.g., build system, specific module functionality)? How does the change work?
  • Impact: Simply stating "RELEASE" is insufficient. All impact sections need to be addressed with "YES" or "NO" and a description if "YES." Even if the answer is "NO," it's helpful to explicitly state it for each item (e.g., "Impact on user: NO"). Consider backward compatibility, particularly if this changes build dependencies.
  • Testing: "CI" is not enough. While CI testing is important, the requirements ask for specific information about the local test setup (host and target details) and actual testing logs before and after the change. What was tested and what were the results? Just saying "CI" doesn't demonstrate that the change works as intended.

The PR needs to be significantly revised to meet the NuttX requirements and provide sufficient context and evidence for reviewers.

Copy link

@cederom cederom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @jerpelea :-)

@cederom
Copy link

cederom commented Dec 19, 2024

CI errors, restarted :-)

@cederom
Copy link

cederom commented Dec 19, 2024

@lupyuen could you please take a look why CI fails? I can not see any specific error reported :-(

Copy link

@cederom cederom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, lets fix the CI build problem first :-)

@lupyuen
Copy link
Member

lupyuen commented Dec 20, 2024

@cederom sim:lua builds OK now, after I clicked "Re-Run All Jobs" (instead of Failed Jobs)
https://github.com/apache/nuttx-apps/actions/runs/12408505693/job/34685151351?pr=2907#step:7:343

Configuration/Tool: sim/lua
  Building NuttX...
  Normalize sim/lua

That's because Fetch Source needs to fetch the latest source from nuttx @ releases/12.8. I discovered this when building sim:lua with Docker:
https://gist.github.com/lupyuen/20415deb88c890c0284b6ba5fc7b3836

sudo docker run \
  -it \
  ghcr.io/apache/nuttx/apache-nuttx-ci-linux:latest \
  /bin/bash
cd
git clone https://github.com/apache/nuttx --branch releases/12.8
git clone https://github.com/jerpelea/nuttx-apps apps --branch bp-2902
pushd nuttx ; echo NuttX Source: https://github.com/apache/nuttx/tree/$(git rev-parse HEAD) ; popd
pushd apps  ; echo NuttX Apps: https://github.com/apache/nuttx-apps/tree/$(git rev-parse HEAD) ; popd
cd nuttx
tools/configure.sh sim:lua
make -j
## NuttX Source: https://github.com/apache/nuttx/tree/925b8b0904dcdd8d2c36b5d0a3fe0f1960e6a5bf
## NuttX Apps: https://github.com/apache/nuttx-apps/tree/fd7b8327f2185358c6bd29018479fdcb7fe7a8bd

sim-02 also builds correctly with Docker:
https://gist.github.com/lupyuen/394aaa99616553921238ab6e164b6614

sudo docker run \
  -it \
  ghcr.io/apache/nuttx/apache-nuttx-ci-linux:latest \
  /bin/bash
cd
git clone https://github.com/apache/nuttx --branch releases/12.8
git clone https://github.com/jerpelea/nuttx-apps apps --branch bp-2902
pushd nuttx ; echo NuttX Source: https://github.com/apache/nuttx/tree/$(git rev-parse HEAD) ; popd
pushd apps  ; echo NuttX Apps: https://github.com/apache/nuttx-apps/tree/$(git rev-parse HEAD) ; popd
cd nuttx/tools/ci
./cibuild.sh -c -A -N -R testlist/sim-02.dat 

Copy link

@cederom cederom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @jerpelea.. and @lupyuen for help with CI :-)

@xiaoxiang781216 xiaoxiang781216 merged commit 972b7c0 into apache:releases/12.8 Dec 20, 2024
23 of 25 checks passed
@jerpelea jerpelea deleted the bp-2902 branch December 20, 2024 01:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants