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

create luac.cross.{integer|float} #2450

Merged
merged 3 commits into from
Aug 11, 2018

Conversation

HHHartmann
Copy link
Member

  • 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/en/*.

<Description of and rationale behind this PR>
As discusse in issue #2443 I changed the makefiles to create luac.cross.integer and luac.cross.float depending on the defined build type as defined for docker builds (and others)

I also added a .gitignore file to local/lua dictionry so that it appears on chekout/clone as local/fs does.

chretes different binaries during integer and float builds.
Also adds local/lua directory which is already supported by tools makefile to build LFS image.
@TerryE
Copy link
Collaborator

TerryE commented Aug 4, 2018

Gregor, I'll look at these later but you will have problems getting this sort of script through Travis as Travis is also missing around with what you are doing and it will get confused. I look later or tomorrow :)

@HHHartmann
Copy link
Member Author

Ah sorry. Didn't come back to check for the Travis results.
I can have a lock myself this afternoon.

BTW the mobile view does not show Travis results at all.

@TerryE
Copy link
Collaborator

TerryE commented Aug 5, 2018

Gregor, my main comment here is driven by the question: why on earth we have an integer build at all?

When I first started working on NodeMCU, typical builds left ~15Kb heap at start-up for both code and data, so getting the simplest usable Lua application required all sorts tricks including doing integer builds because integer build TValues where 8 bytes vs. 16 for float. If you are using LFS, you now have ~45Kb heap at boot for data only and the float TValues are 12 bytes not 16.

In terms of performance, there is bugger-all different between float and integer run-times since the GC costs dominate here. When I do eventually have time to release my Lua 5.3 upgrade, the Lua VM handles int and float as separate subtypes of lua_Number and all TValues are 8-bytes.

IMO, we need to continue integer support because there is a sizeable percentage of application developers that use it, but we should also start gently deprecating it. Hence I suggest that we use luac.cross and luac.cross.int as the two names, so that when we go to Lua 5.3 we will still continue with luac.cross and drop luac.cross.int.

@marcelstoer
Copy link
Member

the mobile view does not show Travis results at all.

Yes, it does - at least on my mobile.
github
Travis even sends a build failure email to the author. However, since most GitHub users are like you and I and chose to anonymize their email addresses (HHHartmann@users.noreply.github.com) we never find out.

tools/Makefile Outdated
@@ -87,3 +94,6 @@ spiffsclean: remove-image
rm -f ./spiffsimg/spiffsimg
rm -f ./spiffsimg/spiffs.lst

#PDIR := ../$(PDIR)
#sinclude $(PDIR)Makefile
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure why you include these two lines, even if commented out.

IMO, the normal reason to include commented out lines is to indicate to developers that they might want to enable these options to add enriched features. In the case of both the luac_cross and spiffimg Makefiles these are using the host toolchain and not the xtensa one so doing the sinclude $(PDIR)Makefile make file magic would actually break the build in this case.

So please don't add these misleading lines 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes that's a leftover of my tries to get the int/float detection in the main Makefile.
Removing!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Gregor, have you got your head around the recursion magic? For a standard library, e.g. modules Root make→app make→modules make→(sinclude) app make→(sinclude) root make, so for the xtensa toolchain stuff all of the hard rules are in the root make driven by a bunch of variables set in the modules make (or equivalent).

The host target makes (luac_cross and spiffsimg) are different in that they do the actual (host)compile and build of the component and don't to CBs so no sinclude statements: Root make→app make→lua make→lua_cross make.

@HHHartmann
Copy link
Member Author

@TerryE Terry I agree with your plans for int builds. Changing names as you suggest.

@HHHartmann
Copy link
Member Author

@marcelstoer Marcel, there is definitively no Travis results for me. Maybe it's due to the different roles we take.

now we have luac.cross and luac.cross.int
@HHHartmann
Copy link
Member Author

Thanks @TerryE My way was a bit harder, not being a Makefile whiz it took me some time to find out ;.)

@marcelstoer
Copy link
Member

there is definitively no Travis results for me. Maybe it's due to the different roles we take.

Makes sense, I keep forgetting that I'm likely seeing more options than others. The GitHub GUI even tries to help me as the relevant section is labeled "Merge this pull request" (-> screen shot) but I didn't realize.

Thanks for your work on this PR.

@marcelstoer marcelstoer added this to the next-release milestone Aug 6, 2018
@HHHartmann
Copy link
Member Author

Thanks for your work on this PR.

It's all been my pleasure.

@TerryE TerryE merged commit 91656c3 into nodemcu:dev Aug 11, 2018
@HHHartmann HHHartmann deleted the luac.cross.{integer/float} branch August 27, 2018 15:31
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.

3 participants