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

sim: add defconfig for testing cmake compilation of quickjs #14693

Merged
merged 2 commits into from
Nov 12, 2024

Conversation

WenLY1
Copy link
Contributor

@WenLY1 WenLY1 commented Nov 8, 2024

Note: Please adhere to Contributing Guidelines.

Summary

Update this section with information on why change is necessary,
what it exactly does and how, if new feature shows up, provide
references (dependencies, similar problems and solutions), etc.

Impact

Update this section, where applicable, on how change affects users,
build process, hardware, documentation, security, compatibility, etc.

Testing

Update this section with details on how did you verify the change,
what Host was used for build (OS, CPU, compiler, ..), what Target was
used for verification (arch, board:config, ..), etc. Providing build
and runtime logs from before and after change is highly appreciated.

@github-actions github-actions bot added Board: simulator Size: S The size of the change in this PR is small labels Nov 8, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 8, 2024

[Experimental Bot, please feedback here]

This PR template does a good job of outlining the information needed for a NuttX contribution. However, it could be improved with a few tweaks to make it clearer and more actionable for contributors.

Here's a revised template:

# NuttX Pull Request

## Summary

* **What problem does this solve?** (bug fix, enhancement, new feature)  Be specific and concise.
* **What code is affected?**  (e.g., drivers/sensors, sched/, net/...)
* **How does this change address the problem?** Briefly explain the technical approach.
* **Related Issue:**  (e.g., `Fixes #1234`, `Part of #5678`)  Link to the NuttX issue, if applicable.
* **Related NuttX Apps Issue/PR:** Link to the NuttX Apps issue or PR, if applicable.

## Impact

Please answer YES or NO for each question and provide a brief explanation if YES.

* **New Feature?** [ ] YES / [ ] NO (If YES, describe the new functionality)
* **User Impact?** [ ] YES / [ ] NO (If YES, how will users be affected? Will they need to change their code or configuration?)
* **Build Impact?** [ ] YES / [ ] NO (If YES, how will the build process be affected?  New dependencies?  New configuration options?)
* **Hardware Impact?** [ ] YES / [ ] NO (If YES, which architectures, boards, or drivers are affected?)
* **Documentation Impact?** [ ] YES / [ ] NO (If YES, does documentation need updating?  Has documentation been updated as part of this PR?)
* **Security Impact?** [ ] YES / [ ] NO (If YES, describe any potential security implications)
* **Compatibility Impact?** [ ] YES / [ ] NO (If YES, describe any backward/forward compatibility or interoperability issues)
* **Other Considerations?** (Anything else reviewers should be aware of?)


## Testing

* **Build Host(s):** OS (e.g., Linux Ubuntu 20.04), CPU (e.g., x86_64), Compiler (e.g., GCC 9.4.0)
* **Target(s):** Architecture (e.g., sim, RISC-V, ARM Cortex-M4), Board:Config (e.g., stm32f4discovery:nsh)

**Verification Steps:**  (Clearly describe the steps taken to test this change. Be specific!)

**Test Results:**

* **Before Change:**

(Paste relevant logs or output demonstrating the issue before the change. If a new feature, describe the expected behavior.)

* **After Change:**

(Paste relevant logs or output demonstrating the fix or new functionality.)




Key improvements:

  • Clearer questions: Rephrased questions in the Impact section to be more direct and specific. Checkboxes added for YES/NO answers.
  • Emphasis on "What problem does this solve?": This encourages contributors to clearly articulate the purpose of their change.
  • Specific code area: Asking for the affected code area helps reviewers quickly understand the scope of the change.
  • Verification Steps: Added a section for explicit test steps. This is crucial for reviewers to understand how the changes were validated and to reproduce the testing if necessary.
  • Before/After Expectations: Encouraged including the expected behavior before the change, especially for new features, or describing the problem being solved. This clarifies the context for the "After Change" results.

This revised template is more structured and provides clearer guidance for contributors, leading to higher-quality pull requests and a smoother review process.

Copy link
Member

@lupyuen lupyuen left a comment

Choose a reason for hiding this comment

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

Later we can rename nsh/defconfig to quickjs/defconfig. Thank you so much for porting QuickJS to NuttX! :-)

@lupyuen
Copy link
Member

lupyuen commented Nov 8, 2024

Please remember to run tools/refresh.sh and commit the updated defconfig. Thanks!

$ tools/refresh.sh --silent sim:nsh
  Normalize sim:nsh
37a38,40
> CONFIG_INTERPRETERS_QUICKJS=y
> CONFIG_INTERPRETERS_QUICKJS_BIGNUM=y
> CONFIG_INTERPRETERS_QUICKJS_FULL=y
39a43
> CONFIG_LIBC_DLFCN=y
51a56
> CONFIG_PIPES=y
59d63
< CONFIG_SCHED_WAITPID=y
65,66d68
< CONFIG_TESTING_OSTEST=y
< CONFIG_PIPES=y
68,76c70
< CONFIG_SYSTEM_POPEN_STACKSIZE=2048
< CONFIG_SYSTEM_POPEN_PRIORITY=100
< CONFIG_LIBC_DLFCN=y
< CONFIG_LDPATH_INITIAL=""
< CONFIG_INTERPRETERS_QUICKJS=y
< CONFIG_INTERPRETERS_QUICKJS_FULL=y
< CONFIG_INTERPRETERS_QUICKJS_BIGNUM=y
< CONFIG_INTERPRETERS_QUICKJS_PRIORITY=100
< CONFIG_INTERPRETERS_QUICKJS_STACKSIZE=8192
---
> CONFIG_TESTING_OSTEST=y
Saving the new configuration file

$ git status
Changes not staged for commit:
	modified:   boards/sim/sim/sim/configs/nsh/defconfig

@WenLY1
Copy link
Contributor Author

WenLY1 commented Nov 8, 2024

@lupyuen Should I created the file quickjs/defconfig and run tools/refresh.sh --silent sim:nsh?

@lupyuen
Copy link
Member

lupyuen commented Nov 8, 2024

Yes please run tools/refresh.sh --silent sim:quickjs thanks!

@WenLY1 WenLY1 force-pushed the qjs_defconfig branch 2 times, most recently from 0aabeb7 to 0b9bbdd Compare November 8, 2024 10:21
@WenLY1
Copy link
Contributor Author

WenLY1 commented Nov 8, 2024

Yes please run tools/refresh.sh --silent sim:quickjs thanks!

Done

@lupyuen
Copy link
Member

lupyuen commented Nov 8, 2024

Can you commit the modified defconfig? Thanks!

$ git clone https://github.com/WenLY1/nuttx --branch qjs_defconfig
$ git clone https://github.com/WenLY1/nuttx-apps apps --branch quickjs_upstream
$ cd nuttx
$ tools/refresh.sh --silent sim:quickjs
$ git status
	modified:   boards/sim/sim/sim/configs/quickjs/defconfig

@WenLY1
Copy link
Contributor Author

WenLY1 commented Nov 11, 2024

Can you commit the modified defconfig? Thanks!

$ git clone https://github.com/WenLY1/nuttx --branch qjs_defconfig
$ git clone https://github.com/WenLY1/nuttx-apps apps --branch quickjs_upstream
$ cd nuttx
$ tools/refresh.sh --silent sim:quickjs
$ git status
	modified:   boards/sim/sim/sim/configs/quickjs/defconfig

Done. :)

@lupyuen
Copy link
Member

lupyuen commented Nov 11, 2024

We'll re-run the CI Checks when nuttx-apps has been updated thanks!

@lupyuen lupyuen changed the title add defconfig for testing cmake compilation of quickjs sim: add defconfig for testing cmake compilation of quickjs Nov 11, 2024
@lupyuen
Copy link
Member

lupyuen commented Nov 11, 2024

Hmmm I think I need to update the CI Testlist to run CMake (instead of normal Make). Lemme check...

Configuration/Tool: sim/quickjs
make[3]: *** [Makefile:72: quickjs] Error 1
make[3]: Target 'context' not remade because of errors.
make[2]: *** [Makefile:55: /github/workspace/sources/apps/interpreters/quickjs_context] Error 2
make[2]: Target 'context_all' not remade because of errors.
make[1]: *** [Makefile:185: context] Error 2
make: *** [tools/Unix.mk:457: /github/workspace/sources/apps/.context] Error 2
make: Target 'all' not remade because of errors.
/github/workspace/sources/nuttx/tools/testbuild.sh: line 385: /github/workspace/sources/nuttx/../nuttx/nuttx.manifest: No such file or directory

https://github.com/apache/nuttx/actions/runs/11774919637/job/32796450018#step:7:152

@lupyuen
Copy link
Member

lupyuen commented Nov 11, 2024

@WenLY1 Could you rebase to master branch while I check on CMake in the Testlist? Thanks!

@WenLY1
Copy link
Contributor Author

WenLY1 commented Nov 11, 2024

@WenLY1 Could you rebase to master branch while I check on CMake in the Testlist? Thanks!

Done

@lupyuen
Copy link
Member

lupyuen commented Nov 11, 2024

@WenLY1 Could you please make this change to tools/ci/testlist/sim-03.dat (so that it will build QuickJS with CMake instead of Make):

Squash the commits, then I will re-run the CI Check. Thanks!

@WenLY1
Copy link
Contributor Author

WenLY1 commented Nov 11, 2024

@WenLY1 Could you please make this change to tools/ci/testlist/sim-03.dat (so that it will build QuickJS with CMake instead of Make):

Squash the commits, then I will re-run the CI Check. Thanks!

Done

@lupyuen
Copy link
Member

lupyuen commented Nov 11, 2024

Thanks @WenLY1! Now the CI Checks won't build sim-03 with QuickJS (because we modified testlist and it became a Complex PR).

So I cloned your branch to my repo and forced it to build sim-03 with QuickJS. Let's check the result here: https://github.com/nuttxpr2/nuttx/actions/runs/11777084493/job/32800846925

@lupyuen
Copy link
Member

lupyuen commented Nov 11, 2024

@WenLY1 Sorry somehow your defconfig reverted to the old version.

https://github.com/apache/nuttx/pull/14693/files

## Should be sorted alphabetically, should not appear at the end.
CONFIG_INTERPRETERS_QUICKJS=y

Could you refresh and recommit the defconfig? Thanks!

$ git clone https://github.com/WenLY1/nuttx --branch qjs_defconfig
$ git clone https://github.com/WenLY1/nuttx-apps apps --branch quickjs_upstream
$ cd nuttx
$ tools/refresh.sh --silent sim:quickjs
$ git status
	modified:   boards/sim/sim/sim/configs/quickjs/defconfig

@lupyuen
Copy link
Member

lupyuen commented Nov 12, 2024

Hmmm the Commits don't look right...
Screenshot 2024-11-12 at 9 43 14 AM

@lupyuen
Copy link
Member

lupyuen commented Nov 12, 2024

If it's too messy to fix the commits, maybe we could close this PR, do a new PR with the 2 changed files? Thanks, sorry for the extra work :-)

@WenLY1
Copy link
Contributor Author

WenLY1 commented Nov 12, 2024

If it's too messy to fix the commits, maybe we could close this PR, do a new PR with the 2 changed files? Thanks, sorry for the extra work :-)

It's ok now :)

@xiaoxiang781216 xiaoxiang781216 marked this pull request as ready for review November 12, 2024 01:54
Copy link
Member

@lupyuen lupyuen left a comment

Choose a reason for hiding this comment

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

CI Check tested OK on Docker with sim-03 and sim:quickjs. Thank you so much! :-)
https://gist.github.com/nuttxpr/27319238463830aa03ff08dffbf34fbb

$ sudo docker run -it \
  ghcr.io/apache/nuttx/apache-nuttx-ci-linux:latest \
  /bin/bash
$ cd
$ git clone https://github.com/WenLY1/nuttx --branch qjs_defconfig
$ git clone https://github.com/apache/nuttx-apps apps ;
$ cd nuttx/tools/ci
$ ./cibuild.sh -c -A -N -R testlist/sim-03.dat 

Cmake in present: sim/quickjs
Configuration/Tool: sim/quickjs
  Cleaning...
  Configuring...
  Select HOST_LINUX=y
  Select HOST_X86_64=y
  Building NuttX...

@xiaoxiang781216 xiaoxiang781216 merged commit 1227203 into apache:master Nov 12, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: CI Area: Tooling Board: simulator Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants