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

interpreters/quickjs/Makefile: fix the build with Make #2993

Merged
merged 1 commit into from
Feb 21, 2025

Conversation

simbit18
Copy link
Contributor

Summary

QUICKJS_URL_BASE in Cmakefile and Makefile was not the same.
Updated the correct url in Makefile.

Impact

Closes apache/nuttx#15712 fixing the build with Make

Testing

./tools/configure.sh -l sim:quickjs
make -j

Github
https://github.com/simbit18/nuttx_test_pr/actions/runs/13306558512/job/37158861055#logs

local on Alpine Linux

qjs

@nuttxpr
Copy link

nuttxpr commented Feb 13, 2025

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the NuttX requirements, although the provided information could be more thorough.

Here's a breakdown of why and suggestions for improvement:

Strengths:

  • Clear Summary: The summary concisely explains the problem and solution.
  • Issue Reference: Linking the issue is crucial for traceability.
  • Testing Evidence: Providing build logs and a screenshot demonstrates testing effort.
  • Impact Description: Clearly states the impact is fixing a build issue.

Weaknesses & Suggestions:

  • Incomplete Summary: While the summary addresses what changed, it lacks detail on how the Makefile was updated. Specify the old and new values of QUICKJS_URL_BASE. Mentioning the functional part affected (build system) would also be helpful.
  • Limited Testing Detail: While logs are provided, more context is needed.
    • Before Change Logs: Show the build failure caused by the incorrect URL.
    • After Change Logs: Ideally, show a successful build including the download and integration of QuickJS. The screenshot isn't very informative on its own.
    • Target Details: Be more specific about the target. "sim:quickjs" is good, but adding information about the host OS and compiler used for the local build would improve reproducibility. Example: "Alpine Linux, GCC 12.2.1".
  • Incomplete Impact Assessment: While the build impact is addressed, consider other potential impacts:
    • Documentation: If any documentation refers to the old URL, it needs updating.
    • Compatibility: Is this change backward compatible? If someone had manually worked around the issue before, could this change cause problems for them?
  • Missing "Before Change" Logs: Showing the build failure before the change is vital for demonstrating the fix's effectiveness.

Example of Improved Testing Section:

Testing

I confirm that changes are verified on local setup and works as intended:

* Build Host(s): Alpine Linux, GCC 12.2.1
* Target(s): sim:quickjs

Testing logs before change:

make -j
...
Downloading from incorrect URL...
...
Error: Download failed / Could not find QUICKJS
...
make: *** [...] Error 1


Testing logs after change:

make -j
...
Downloading from https://new.correct.url/quickjs...
...
Successfully downloaded QUICKJS
...
Building QUICKJS...
...
Linking QUICKJS...
...
Build complete.


Github Actions Build Log: [link to specific section showing successful download and build]


By addressing these points, the PR will be even stronger and easier for reviewers to assess.

@acassis
Copy link
Contributor

acassis commented Feb 13, 2025

@simbit18 is there some TAG that you can use? Using the "master" probably will make it break soon

@simbit18
Copy link
Contributor Author

simbit18 commented Feb 13, 2025

@simbit18 is there some TAG that you can use? Using the "master" probably will make it break soon

Hi Alan, for TAG we need to change the Makefile from curl to git clone .
Cmakefile also needs to be modified.

https://github.com/bellard/quickjs

I will do that in another PR.

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

  • There is inconsistency between QUICKJS_VERSION=master and VERSION=\"2020-11-08\". I think we should use release not master..?
  • Why not use VERSION=\"$(QUICKJS_VERSION)\" as before?
  • Please add git commit -s signature to git commit :-)

@simbit18
Copy link
Contributor Author

simbit18 commented Feb 14, 2025

Hi @cederom

Make
Before this PR apache/nuttx#14693 the sim:quickjs build was only with make the reference package was this quickjs-2020-11-08.tar.xz (QUICKJS_VERSION and VERSION were the same)

QUICKJS_VERSION is used in the construction of the url
url -> https://bellard.org/quickjs/quickjs-$(QUICKJS_VERSION).tar.xz

Cmake
After this apache/nuttx#14693 and PR #2827 the sim:quickjs build was moved to cmake with reference to the master package of the quickjs repository also modifying the patch.
url -> https://github.com/bellard/quickjs/archive/refs/heads/master.zip

So sim:quickjs is built correctly on Ubuntu with Docker because cmake uses different sources. Obviously if we go through the build with make it fails !!!

In fact here is what happens on our NuttX mirror macOS (sim-03) uses Make

====================================================================================
Configuration/Tool: sim/quickjs
2025-02-14 02:41:31
------------------------------------------------------------------------------------
  Cleaning...
  Configuring...
  Building NuttX...
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed

  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
100  734k  100  734k    0     0   894k      0 --:--:-- --:--:-- --:--:--  895k
make[3]: *** [quickjs] Error 1
make[3]: Target `context' not remade because of errors.
make[2]: *** [/Users/runner/work/nuttx/nuttx/sources/apps/interpreters/quickjs_context] Error 2
make[2]: Target `context_all' not remade because of errors.
make[1]: *** [context] Error 2
make: *** [/Users/runner/work/nuttx/nuttx/sources/apps/.context] Error 2
make: Target `all' not remade because of errors.
/Users/runner/work/nuttx/nuttx/sources/nuttx/tools/testbuild.sh: line 385: /Users/runner/work/nuttx/nuttx/sources/nuttx/../nuttx/nuttx.manifest: No such file or directory
  [1/1] Normalize sim/quickjs
====================================================================================

https://github.com/NuttX/nuttx/actions/runs/13326598748/job/37221187619#logs

The purpose of my PR is to restore the build with Make and resolve the build on the NuttX mirror.

As @acassis has rightly as pointed out it is necessary to have a reference TAG.

With a new PR I will fix Cmakefile and Make to avoid future breakage.

QUICKJS_URL_BASE in Cmakefile and Makefile was not the same.
Updated the correct url in Makefile.

fix apache/nuttx#15712

To avoid future breakage, used the URL with last commit ID

Current version 2024-02-14 as found in the VERSION file https://github.com/bellard/quickjs/blob/master/VERSION

Signed-off-by: simbit18 <[email protected]>
@simbit18
Copy link
Contributor Author

simbit18 commented Feb 17, 2025

@cederom @acassis @lupyuen @jerpelea @xiaoxiang781216

I have updated the PR
To avoid future breaks, use the URL with the last current commit ID

Current version 2024-02-14 as found in the VERSION file https://github.com/bellard/quickjs/blob/master/VERSION

Signed the commit @cederom I warn you If I receive spam emails they will be redirected to your email !!!! :)

qjs_commit

@simbit18
Copy link
Contributor Author

simbit18 commented Feb 21, 2025

Here is also the PR for Cmake #3001

@acassis @cederom Do you have any other questions or concerns about this PR?

@acassis acassis merged commit 71ff607 into apache:master Feb 21, 2025
37 checks passed
@simbit18 simbit18 deleted the simbit18-20250212 branch February 21, 2025 16:30
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.

[BUG] Make: sim:quickjs fails to build
7 participants