Remove the built-in print function

[akosthekiss]

This PR is a follow-up to #1737. For now, it contains multiple commits so that
if discussion concludes that some of the changes should go into a separate PR,
it will be easy to drop those changes.

NOTE 1: This is a breaking change!!!
NOTE 2: This patch still leaves several targets without a JS print implementation.

Summary of changes:

• Remove the built-in print function

  The built-in print is removed from jerry-core, but an external
  print implementation is added to jerry-main. From now on, all
  embedders of the engine have to implement their own print if they
  need such a functionality.

• Call external print handler in jerry-main

  For printing results in REPL mode, call the external print
  handler directly instead of looking up the print function
  registered into the global object. (The two are the same, but the
  indirection is not needed anymore.)

• Remove jerry_port_console from the port API

  The default port is updated, i.e., the implementation of
  jerry_port_console is removed. Additionally, all references to
  jerry_port_console in jerry-main are replaced by printf.

• Speculatively removing jerry_port_console from all non-default targets

  Most targets implemented jerry_port_console for the sake of the
  engine only; in those targets the removal was trivial. Where the
  function was called from the embedder application as well, the
  calls were replaced with equivalents (e.g., printf, printk).

[LaszloLango]

Please update docs/05.PORT-API.md too.

[akosthekiss]

Docs updated

[zherczeg]

LGTM.

I am happy that port_console is gone, I never liked it.

[pfalcon]

but an external print implementation is added to jerry-main

Sorry, I don't see anything being added to to jerry-main. I see something being added to jerry-main/main-unix.c. Is the intention of this patch is to really do all the following:

1. Leave bunch of ports completely broken (nice show-off of JerryScript capabilities).
2. Make every port to copy-past print() implementation from unix port.
3. p.2 is actually too much effort. To copy-paste, someone first needs to look it up it in unix port. But in the "challenge accepted" manner people won't waste time with studying other ports, but will code up adhoc, buggy implementations of print themselves.

If the above weren't intentions of this patch, that they are very obvious risk factors, about which someone should have thought.

Summing up: -1. As the maintainers of the project, who accepted ports patches into mainline, please kindly perform your refactors in a way to not break these ports (that's the reason people contribute code upstream, and a kind of social contract between maintainers and contributors). Thanks.

[martijnthe]

Maybe a print binding can be added into the new jerry-ext/jerry-util to avoid the copy & pasting?

Question: are the various ports being built & tested as part of the CI?

[zherczeg]

Most ports are not tested. Ports should have an owner who maintains the port specific testing.

[pfalcon]

Testing, yeah, that's a known trait of the project (and fairly speaking, of majority of open-source projects which don't have full-fledged CI, for multiple platforms etc.). But relying on external contributors for testing changes in ports is different from making changes which guaranteedly break ports. The ports are in-tree exactly to prevent cases like that, that removing something breaks a lot of users.

[akosthekiss]

@pfalcon Frankly, I'm somewhat offended by your comment.

1. The first comment is merely splitting hairs. main-unix.c is in jerry-main. Allow me to refer to coarse-grained components (like jerry-core or jerry-main) instead of specific files, sometimes.

2. The description of the PR is referring to the issue [Discussion] Are JS print and C jerry_port_console functions needed? #1737, where the idea has been introduced, pros and cons have been analyzed, and people from 3 different teams (Intel, Samsung, U-Szeged) have agreed that we should go forward with implementation. (That may already cover some 7 targets of JerryScript: unix, curie_bsp, zephyr, nuttx-stm32f4, riot-stm32f4 + the repo-external iot.js.)

3. Both this PR and the corresponding issue acknowledge that, as the PR stands now, the status of several targets is broken. Therefore the issue explicitly asks for feedback from port maintainers.

BTW, the esp8266 port should be working all right after this patch (in whatever form it might land) as it already has its own print implementation.

That being said, I kind of agree with @martijnthe to provide default/template implementations for several widespread but non-standard functions (potentially not only for print but for gc and assert, too) in the now-forming jerry-util utility library, from which everyone can cherry-pick whatever life-simplifying function s/he may need.

[LaszloLango]

LGTM

[akosthekiss]

I've prefixed PR title with WIP to highlight that it should NOT land yet, even though some maintainers have approved it. I'm really looking for port maintainer feedback before landing this into master.

[pfalcon]

@akosthekiss , (as a co-maintainer of Zephyr port), I recently find it hard to follow JrS churn. Some changes I'd expect to spent good weeks in review regarding whether they should be part of the project, e.g. #1725 suddenly (perhaps to everyone but the core team of Intel, Samsung, U-Szeged) get committed in just few days. So, you never know whether something is a very draft proof of concept patch to be elaborated during next month, or something to be pushed tomorrow. So, please excuse my alarming tone.

I also hope you didn't treat my response as trying to challenge maintainers' decision of removing "print" from the core engine (to moving it outside of core). My review is based on the exact current version of the patch.

The first comment is merely splitting hairs. main-unix.c is in jerry-main. Allow me to refer to coarse-grained components (like jerry-core or jerry-main) instead of specific files, sometimes.

Apparently, you were referring to just subdirectories, whereas I treated names exactly as the "components", sorry for the confusion. And treating "jerry-main" as a component, adding a static function to file named main-unix.c, clearly showing it's port-specificity (also confirmed by other clearly unix-specific function in this module), and thus not helping "component architecture" at all.

BTW, the esp8266 port should be working all right after this patch (in whatever form it might land) as it already has its own print implementation.

Great to hear! And of course, any coincidences with me talking "will code up adhoc, buggy implementations of print themselves" are random. (Feel free to leave "buggy" on my conscience - until we'll start to get CVEs for IoT stuff and CNN will talk about IoT botnets, we indeed can only hope that code/effort duplication ain't that bad after all).

So, with you mentioning 7 other targets as (almost?) working, it seems that Zephyr port would be the only one actually broken by this change. And I can explain why it would be: because at one point I decided that printing "error" on error ain't enough, what will users think about JerryScript, if in addition to (at that time) lack of function name reporting, line number reporting on error, it's seemingly unable to report a specific error at all? So, I took that code from main-unix and copy-pasted it into Zephyr port (there were problems with code reuse before as there're now). And the port would be broken now because of my good intentions to present JerryScript to user better rather than worse. So, I hope you see my dilemma now: unless maintainers would take care to maintain ports at a reasonable level (apply refactors, etc.), then doing it quick-and-dirty would be actually better than more thorough, because otherwise port will be potentially broken again and again, and somebody from community constantly need to watch after it (and community has much less resources than the parties mentioned above). Hope that explains my concern and sorry again if from your point of view this doesn't represent things straight (I'm just sharing my PoV as an external contributor).

[pfalcon]

That being said, I kind of agree with @martijnthe to provide default/template implementations for several widespread but non-standard functions (potentially not only for print but for gc and assert, too) in the now-forming jerry-util utility library

Ah, and sure, that would be an implied solution. Except if I formulated it, I'd say "please just separate print() implementation into a separate source file in jerry-main, and at least try to patch ports to use that file, the community will help to actually test and tweak that". As you see, I'd think about putting it in jerry-main, while you talk about jerry-util, which only shows that maintainers know better how to do. So, I may just wonder why even a draft patch, but had it as static for unix port only.

[akosthekiss]

@pfalcon Re: "jerry-main", that subdir is actually just yet-another-port/target, like targets/zephyr, targets/mbedos5, or targets/nuttx-stm32f4. There are two reasons for having it in the root directory instead of under targets: 1) Legacy. This was the very first (or one among the firsts) app that embedded the engine. 2) Practicality. This is a distinguished app that is the most versatile and easiest to use/test/validate, as it works, or should work, on almost everyone's desktop without any extra hardware -- as long as it is posix-ish, e.g., Linux and OSX, but I've seen it running on the Linux subsystem of Windows 10, too. I acknowledge that right now the PR only handles this jerry-main target properly. There are still todos ahead -- modulo port maintainer feedback. (Either we conclude that it is the responsibility of this PR to handle all targets, or some port maintainers want to deal with the situation by themselves in follow-up patches.)

Re: "buggy code", actually, no disagreement here. "The s in IoT stands for security". We all have to work on that...

Re: "7 targets working", I wasn't trying to suggest that 7 targets were already in a working state, only that maintainers of cca. 7 targets (my guesstimate) were (should have been) aware of the change that's ahead, and voted for it.

Re: "maintain ports at a reasonable level", note that the PR already tries to do some work on/for the ports. The port apps should be building fine, as all C-level changes have been propagated there (speculatively, true). As for the availability of print in the JS context see "I acknowledge..." and "still todos ahead" above.

Re: "print() implementation into a separate source file in jerry-main", that couldn't work IMHO. Right now, I'm not aware of any solution for sharing files among targets. (See above, jerry-main is just another target.) jerry-util could be such a now-missing solution.

[pfalcon]

@akosthekiss : Thanks. I hope a good solution will be found. I for one ready to do any needed legwork for Zephyr port, though as I mentioned above, I'm starting to have doubts what's the best direction for that. It's clear that a raw JerryScript port to any platform isn't useful for much, except probably as a demo. But demo also should present project faithfully and from a good side. I'd think that project maintainers should be interested in that, and with many great developments happening in JrS, some attention to ports may be warranted too.

[jiangzidong]

In the curie-bsp port, line141 in this file

   jerry_value_t print_func_name_val = jerry_create_string ((jerry_char_t *) "print");
   print_function = jerry_get_property (global_obj_val, print_func_name_val);

We refer the main-unix and get the print function from global. Then call the print on every ret of repl.
So we need to paste the new implementation of print_handler here and register_js_function (provide print in js world), and directly call print_handler on the ret of repl

Do you want to do it in this patch? or I can post a following one after it lands

[akosthekiss]

I've added a commit on top of the PR. It shows how common external function handlers could be moved to jerry-ext. (As jerry-ext is not in master yet, this is all just a concept.) Feedback is most welcome.

(Especially on jerryx_extfunc_print, as it required an extra indirection for character printing. And whether a register_js_function-like registering helper would also be needed.)

[zherczeg]

I don't really like the jerry-ext/extfunc/ directory name. Common (jerry-ext/common-js-funcs) or non-standard JS function (jerry-ext/extra-js-funcs) would be a better name. Anything that represents its purpose (JS functions which are not part of the standard but generally useful).

[akosthekiss]

For me, extfunc stood for "external functions" (not extra). I'm happy to change, but let's look for a name that's low on spaces, hyphens, and underscores. jerryx_common_js_funcs_ prefix for all the functions will be hard to use. (It seemed to me that the jerryx_XXX_ prefix and the jerryscript-ext/XXX.h names should highly correlate.)

[martijnthe]

How about splitting these up?

jerry-ext/print/print.c
jerry-ext/gc/gc.c
jerry-ext/assert/assert.c
jerry-ext/include/jerryscript-ext/print.h
jerry-ext/include/jerryscript-ext/gc.h
jerry-ext/include/jerryscript-ext/assert.h

[akosthekiss]

I wonder why scattering those functions into separate subdirectories and headers (sort of: separate sub-namespaces)? I.e., "args" compiles everything related to argument handling, validation, conversion, etc. Should common but non-standard functions be not kept together but each split to a different namespace? (If it's only about not pulling in unnecessary implementations into the app binary, the linker will do that automatically if the implementations are in separate sources, even if the declarations are in the same header. Declared but unreferenced functions will not cause the linking of the defining objects.)

[zherczeg]

This isn't an easy question. If we put everything into one file, it can quickly grow too large, containing a lot of unrelated stuff. So I like the proposal from @martijnthe. This would be consistent with the current plans.

[zherczeg]

Would it be a good idea to simply use?

jerry-ext/include/jerryscript-ext/print.h -> jerry-ext/print.h

[akosthekiss]

@zherczeg Please, elaborate. What do you mean by current plans? And what do you mean by unrelated stuff and quickly grow too large? Are there any other common but non-standard functions we'd like to provide implementation for? If they are so non-standard, then they will most probably be app-specific and not useful for many. More importantly, we will not be able to add them to the library if they have any dependency other than jerry-core (and libc+libm, perhaps). Note that even print has issues (see proposed implementation here, topmost commit): we cannot even rely on printf which is a libc function (because of port limitations)!

As for the path renaming, I'm not sure I get it. jerry-ext/include is the dir for public headers. And all jerry-ext extensions are to be used by users as #include "jerryscript-ext/XXX.h". This means that all such headers will have to be under jerry-ext/include/jerryscript-ext in the source tree. (IIRC there was a discussion about this in #1740)

[akosthekiss]

I've rebased the patch set to latest master. I've split out the common external function implementations into a separate PR (#1787) as it seems that there will be a longer and somewhat unrelated discussion on that topic. This PR is still in WIP state (IMHO) as we should discuss whether all component of its patch set should be kept. My main question is: shall we keep and land the topmost commit (jerry_port_console removal from targets)? The risks are that the change is benevolent but speculative, and still leaves several targets broken as it does not implement the print handler for them.

[zherczeg]

I definitely want to remove print from Jerry, so lets start with patch 1.

[LaszloLango]

I vote for the removal of jerry_port_console removal from targets.

[akosthekiss]

I've updated the PR. Related commits got merged, which leaves now two commits only:

• one that removes print from jerry-core, jerry_port_console from the port API, and applies all related changes to the docs, the default port, jerry-main, and the unit tests, and
• one that speculatively updates the ports (recently added/updated ports included).

Until now, there was one vote for landing the first commit only, and one for the whole bundle. Any other opinions?

[LaszloLango]

LGTM

[akosthekiss]

I've removed the WIP prefix from the title of this PR. As barely any feedback was received, I'll rebase the commits to latest master, merge them, and land in the coming 24 hours, unless anyone speaks up.