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 modules struct, bit, color_utils and sjson to luac.cross #3230

Merged
merged 13 commits into from
Feb 25, 2024

Conversation

HHHartmann
Copy link
Member

@HHHartmann HHHartmann commented Jul 24, 2020

Added for linux build and MSC. Excluded MinGW as I don't have an installation. But hope I did not break that build.

Partially Fixes #3229.

  • This PR is for the dev branch rather than for master.
  • This PR is compliant with the other contributing guidelines as well (if not, please describe why).
  • I have thoroughly tested my contribution.
  • The code changes are reflected in the documentation at docs/*.

<Description of and rationale behind this PR>
Help creating tests and allow usage of known functionality on the host.

app/modules/color_utils.c Outdated Show resolved Hide resolved
@HHHartmann
Copy link
Member Author

Sure it compiles.
I just thought there might be defines which would default to something different later on.
BTW the sjson.h and .c have many includes in common. Should I remove them and in which file

@TerryE
Copy link
Collaborator

TerryE commented Jul 24, 2020

IMO, a module -- if it is implemented in a single file -- only needs a .h header if it exports a publicly callable interface. If it doesn't then you have to question why it has one at all.

A header file should only include other headers if that are needed to compile a source that includes it. For example if it's prototypes use a type whose typedef is in another file, though strictly they should use #include_once, not that any do.

@HHHartmann
Copy link
Member Author

I don't know what #include_once means in the C realm. But all includes have guards against double inclusion.

@TerryE
Copy link
Collaborator

TerryE commented Jul 29, 2020

It's a mindfart from my PHP days. 🤣

@HHHartmann HHHartmann force-pushed the Add-modules-to-luac.cross branch 2 times, most recently from 3d102c7 to e76697b Compare August 31, 2020 19:44
@TerryE
Copy link
Collaborator

TerryE commented Sep 11, 2020

Gregor, whilst I think on:

  1. luac.cross must be compilable and executable with the TEST=1 option. This enables the Lua Test Suite additions, and this also adds extra integrity checking on memory allocation.

  2. If we are going to add these then it make sense to do so optionally, e.g. have a LUAC_EXTRA option.

  3. I am currently doing a lot of rework of luac.cross to support First cut of new LFS for initial review #3272. Can we hold off on this PR until after this has landed?

@marcelstoer marcelstoer added this to the Next release milestone Sep 13, 2020
@nwf nwf mentioned this pull request Sep 26, 2020
4 tasks
@nwf
Copy link
Member

nwf commented Sep 27, 2020

Can we add pipe to the list of modules available in luac.cross?

@TerryE
Copy link
Collaborator

TerryE commented Sep 28, 2020

Can we add pipe to the list of modules available in luac.cross?

@nwf the HOST environment contains a minimal emulation of the subset of the platform interface sufficient to allow the Lua core and the small number of supported modules to build and to be callable within the luac.cross -e execution environment. Pipe uses the task interface, so I would need to emulate this as well. This would require a bit of work. In essence, we would probably need to add a node.js-style task scheduler. I think that this counts as a separate task in itself that needs its own cost-benefit analysis.

@TerryE
Copy link
Collaborator

TerryE commented Sep 28, 2020

Nathaniel, BTW, this isn't a -1 to your suggestion, more that it isn't a small change and so we should look at it in the wider context of perhaps providing a sensible subset NodeMCU environment for host use.

@marcelstoer marcelstoer removed this from the Next release milestone Nov 3, 2020
@HHHartmann HHHartmann marked this pull request as draft December 29, 2020 12:07
@HHHartmann HHHartmann force-pushed the Add-modules-to-luac.cross branch 4 times, most recently from e23e8e8 to 19699a4 Compare January 10, 2021 10:18
@HHHartmann HHHartmann marked this pull request as ready for review January 11, 2021 18:26
@HHHartmann
Copy link
Member Author

Three questions about the implementation remain:

  1. Do we need to have this optional as Terry suggests?
  2. pipe currently is added without the possibility of the callback due to a lack of task.post
    Should we just do nothing or throw an error if it is used anyways (task.post is implemented empty for host builds)
  3. I changed luac.cross 5.1 as it passed the filename as parameter to a lua script which interferes with the convention to allow passing an optional NTest instance to a testfile. Is that OK? Its that way on 5.3 anyways.

How should I proceed?

  • currently pixbuf is not added to the win build due to using ssize_t and dynamic sized arrays
    should we go without for now or fix it?
  • Should I add pixbuf host tests in this PR?

@HHHartmann
Copy link
Member Author

Anybody want to vote for this PR
It helps testing Lua programs on the host by adding several hardware unrelated modules to the luac.cross compiler which can also be used to execute Lua programs on the host.
I am using this for years now and it speeds up dev in some cases.

Added for linux build and MSC. Excluded MinGW as I don't have an installation. But hope I did not break that build.
@nwf
Copy link
Member

nwf commented Feb 18, 2024

Looks OK by me; if CI's happy, I'm happy. <3

@marcelstoer marcelstoer merged commit a492a31 into nodemcu:dev Feb 25, 2024
18 checks passed
@HHHartmann
Copy link
Member Author

Thanks guys. Happy to see this merged.

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.

None yet

4 participants