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

add cmake compilation scripts for quickjs #2827

Merged
merged 2 commits into from
Nov 11, 2024

Conversation

WenLY1
Copy link
Contributor

@WenLY1 WenLY1 commented Nov 7, 2024

Summary

add cmake compilation scripts for quickjs

Impact

nop

Testing

cd nuttx
cmake -B build -DBOARD_CONFIG=sim:nsh
cmake --build build -t menuconfig and enable following configs: CONFIG_PIPES, CONFIG_SYSTEM_POPEN, LIBC_DLFCN, CONFIG_INTERPRETERS_QUICKJS, CONFIG_INTERPRETERS_QUICKJS_BIGNUM
cmake --build build

@nuttxpr
Copy link

nuttxpr commented Nov 7, 2024

[Experimental Bot, please feedback here]

No, this PR likely does not fully meet the NuttX requirements based on the provided information. Here's why:

  • Insufficient Summary: While it states what was changed (added CMake scripts for QuickJS), it lacks crucial details:

    • Why is this change necessary? Is it to improve build times, simplify the build process, or address a specific issue?
    • How does it work? Does it replace existing build mechanisms? How does it integrate with the NuttX build system? What files are involved?
  • Incomplete Impact Assessment: "nop" is not acceptable. While the impact might be minimal, the submitter must explicitly consider and address each point:

    • Does this affect users? Will they need to change how they build or use QuickJS?
    • Does this affect the build process itself beyond just adding the CMake option?
    • Does it affect any hardware or specific architectures?
    • Does it require documentation updates? Even if the answer is no, it should be stated explicitly.
    • Security, compatibility, etc., all need explicit "NO" or a detailed explanation if "YES."
  • Inadequate Testing: The testing description is very superficial. It doesn't show what was tested. Just building and enabling the configuration options is not sufficient. There needs to be evidence that QuickJS actually works within NuttX after this change. The logs are empty, providing zero evidence of functional testing. Specific test cases and their results before and after the change are required. Information on the build host is also missing.

To meet the requirements, the PR needs the following:

  • Expanded Summary: Explain the rationale for the change and provide more technical details on how the CMake scripts are integrated.

  • Detailed Impact Assessment: Explicitly address each impact category, even if the answer is "NO."

  • Thorough Testing Information:

    • Specify the build host operating system, CPU architecture, and compiler version.
    • Provide specific test cases executed, including expected outcomes.
    • Include actual "before" and "after" logs demonstrating the functionality of QuickJS both before the change (presumably using the old build system) and after using the new CMake approach. These logs should show the QuickJS interpreter being invoked and producing expected output.

Without these improvements, the PR is likely to be rejected or require significant revisions.

@lupyuen
Copy link
Member

lupyuen commented Nov 7, 2024

Sorry I have to cancel the CI Check, there's a problem with CI Test:

@cederom
Copy link

cederom commented Nov 7, 2024

I have restarted the CI task, is that okay? :-P

@lupyuen
Copy link
Member

lupyuen commented Nov 7, 2024

@cederom Sure but sim-01 will run CI Test very slowly, and after 6 hours, GitHub will kill it anyway due to timeout. I was chatting about it here: apache/nuttx#14680

@lupyuen
Copy link
Member

lupyuen commented Nov 7, 2024

And we're trying to fix it now: #2829

@cederom
Copy link

cederom commented Nov 7, 2024

ACK! Sorry, task cancelled :-)

@lupyuen
Copy link
Member

lupyuen commented Nov 7, 2024

@cederom The (workaround) fix has just been merged: #2829

Hopefully all jobs in NuttX Repo and NuttX Apps won't hang any more :-)

@lupyuen
Copy link
Member

lupyuen commented Nov 7, 2024

@cederom Ack sorry my mind is slow - If you need to restart any of the jobs I cancelled earlier, please click "Restart All Jobs", not "Restart Failed Jobs". This will rerun Fetch-Source to pick up the latest fixes from NuttX Apps. Thanks :-)

@cederom
Copy link

cederom commented Nov 7, 2024

Please restart yourself @lupyuen when ready :-) Thank you for the hint on restart-all to re fetch all updates!! :-)

@cederom
Copy link

cederom commented Nov 7, 2024

@WenLY1 I have problem testing this PR locally.. my first cmake steps in NuttX.. could you please advise? :-)

(venv3.11embedded) uname -a
Linux octagon 5.15.0 FreeBSD 13.3-RELEASE-p7 GENERIC x86_64 GNU/Linux

Looks like cmake tries to create /include directory on your branch why?

(venv3.11embedded) rm -rf build/

(venv3.11embedded) git checkout WenLY1/quickjs_upstream
Updating files: 100% (27852/27852), done.
Note: switching to 'WenLY1/quickjs_upstream'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:

  git switch -c <new-branch-name>

Or undo this operation with:

  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at 7191a587c2 update quickjs patch for cmake compilation


(venv3.11embedded) git branch
* (HEAD detached at WenLY1/quickjs_upstream)
  br_wjy_kernel_sotest_devminor
  master

(venv3.11embedded) cmake -B build -DBOARD_CONFIG=sim:nsh
CMake Warning (dev) in CMakeLists.txt:
  No project() command is present.  The top-level CMakeLists.txt file must
  contain a literal, direct call to the project() command.  Add a line of
  code such as

    project(ProjectName)

  near the top of the file, but after cmake_minimum_required().

  CMake is pretending there is a "project(Project)" command on the first
  line.
This warning is for project developers.  Use -Wno-dev to suppress it.

-- The C compiler identification is GNU 12.2.0
-- The CXX compiler identification is GNU 12.2.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
CMake Error at CMakeLists.txt:41 (file):
  file problem creating directory: /include


CMake Error at audioutils/CMakeLists.txt:21 (nuttx_add_subdirectory):
  Unknown CMake command "nuttx_add_subdirectory".


CMake Warning (dev) in CMakeLists.txt:
  No cmake_minimum_required command is present.  A line of code such as

    cmake_minimum_required(VERSION 3.25)

  should be added at the top of the file.  The version specified may be lower
  if you wish to support older CMake versions for this project.  For more
  information run "cmake --help-policy CMP0000".
This warning is for project developers.  Use -Wno-dev to suppress it.

-- Configuring incomplete, errors occurred!
See also "/XXX/nuttx/build/CMakeFiles/CMakeOutput.log".
(venv3.11embedded) rm -rf build/

There is no problem on nuttx master all works fine:

(venv3.11embedded) git checkout master
Updating files: 100% (27852/27852), done.
Previous HEAD position was 7191a587c2 update quickjs patch for cmake compilation
Switched to branch 'master'
Your branch is up to date with 'origin/master'.

(venv3.11embedded) cmake -B build -DBOARD_CONFIG=sim:nsh
-- Initializing NuttX
  Select HOST_LINUX=y
  Select HOST_X86_64=y
--   CMake   3.25.1
--   Board:  sim
--   Config: nsh
--   Appdir: /XXX/nuttx-apps
-- The C compiler identification is GNU 12.2.0
-- The CXX compiler identification is GNU 12.2.0
-- The ASM compiler identification is GNU
-- Found assembler: /usr/bin/cc
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Configuring done
-- Generating done
-- Build files have been written to: /XXX/nuttx/build

@WenLY1
Copy link
Contributor Author

WenLY1 commented Nov 8, 2024

@cederom Thanks for your testing work. I tested the command cmake -B build -DBOARD_CONFIG=sim:nsh under the directory nuttx, it seems that you are testing this command under the directory nuttx-apps?

@cederom
Copy link

cederom commented Nov 8, 2024

No, under nuttx/ :-) And in the same location after checkout to master build works fine..

@WenLY1
Copy link
Contributor Author

WenLY1 commented Nov 8, 2024

@cederom Sorry, the testing steps is not clear. This PR is for nuttx-apps, could you apply this PR under directory nuttx-apps and then test the command cmake -B build -DBOARD_CONFIG=sim:nsh under the directory nuttx?

@lupyuen
Copy link
Member

lupyuen commented Nov 8, 2024

@WenLY1 For easier testing: Maybe we can create a new defconfig in NuttX Repo that will build sim:nsh with QuickJS? Please keep the defconfig in a Draft PR, so it won't be accidentally merged. Thanks!

@WenLY1
Copy link
Contributor Author

WenLY1 commented Nov 8, 2024

@lupyuen Sure. The defconfig is given in apache/nuttx#14693

@lupyuen
Copy link
Member

lupyuen commented Nov 8, 2024

Thanks @WenLY1! Sorry I have a compile error on macOS, did I miss something? (I'll retest on Ubuntu)

$ git clone https://github.com/WenLY1/nuttx --branch qjs_defconfig
$ git clone https://github.com/WenLY1/nuttx-apps apps --branch quickjs_upstream
$ cd nuttx
$ cmake -B build -DBOARD_CONFIG=sim:nsh
$ cmake --build build
/tmp/241108/nuttx/fs/vfs/fs_pseudofile.c:99:3: error: incompatible function pointer types initializing 'ssize_t (*)(struct file *, const struct uio *)' (aka 'long (*)(struct file *, const struct uio *)') with an expression of type 'int (struct inode *)' [-Wincompatible-function-pointer-types]
   99 |   pseudofile_unlink,   /* unlink */
      |   ^~~~~~~~~~~~~~~~~

https://gist.github.com/lupyuen/784f0c9bd84e5c009e264664017723b2

Update: Yep it works OK on Ubuntu. Let's ignore macOS thanks!

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.

Tested OK on Ubuntu. Thank you so much for contributing QuickJS to NuttX! :-)
https://gist.github.com/nuttxpr/966a3ee6c05ff98727d6d001ea14d738

$ git clone https://github.com/WenLY1/nuttx --branch qjs_defconfig
$ git clone https://github.com/WenLY1/nuttx-apps apps --branch quickjs_upstream
$ cd nuttx
$ cmake -B build -DBOARD_CONFIG=sim:nsh
$ cmake --build build
$ ./build/nuttx 
NuttShell (NSH)
nsh> qjs
QuickJS - Type "\h" for help
qjs > console.log("Hello World")
Hello World

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 @WenLY1 :-)

@WenLY1 WenLY1 marked this pull request as draft November 11, 2024 03:04
@WenLY1 WenLY1 marked this pull request as ready for review November 11, 2024 08:38
@WenLY1
Copy link
Contributor Author

WenLY1 commented Nov 11, 2024

@xiaoxiang781216 xiaoxiang781216 merged commit 4a1c40a into apache:master Nov 11, 2024
25 checks passed
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