Turn port implementations into proper libraries #1777

akosthekiss · 2017-04-24T10:52:08Z

This is the first implementation for #1776 to showcase the idea presented there.

Note that this is a breaking change!

If someone used cmake with the PORT_DIR option, it will not work anymore.
An additional library (i.e., the port implementation library) might need to be
linked to applications that link to jerry.
Some default port implementation-specific function names changed.

However, most probably it will not break most uses cases.

No in-tree targets use the PORT_DIR cmake option.
Many in-tree targets implement the port API within the engine-embedding application,
so there will be no need to link anything else but jerry-core, just like before.
No in-tree targets use the jobqueue port API.

This commit changes the concept of JerryScript port implementations
from a simple directory of C source files (which get injected among
the sources of jerry-core) into a proper static library (which
may be linked to an application together with jerry-core). As a
consequence, this commit introduces a new library to the
JerryScript component architecture: the sources of the default port
implementation form jerry-port-default.

Changes in more detail:

The sources in targets/default are moved to jerry-port/default
and are turned into a proper static library.
- Actually, the default port implementation has two library
  variants, one that implements the bare minimum only
  (jerry-port-default-minimal) and one that has some extra
  functionalities specific to this implementation (the "full"
  jerry-port-default).
- The new libraries have an interface header in
  jerry-port/default/include, which extends the common
  jerryscript-port.h API with functions specific to these
  libraries.
- All non-standard port functions have now the
  jerry_port_default_ prefix (this affects jobqueue_init and
  jobqueue_run).
- The jobqueue implementation functions became config macro
  independent: it is now the responsibility of the linker to pick
  the needed objects from the library, and omit those (e.g.,
  jobqueue-related code) that are not referenced.
- Build of the libraries can be controlled with the new
  JERRY_PORT_DEFAULT cmake option.
The cmake option PORT_DIR is dropped, and PORT_DIR/*.c is not
appended to jerry-core sources.
- Instead, the jerry tool of jerry-main links to
  jerry-port-default, while jerry-minimal links to
  jerry-port-default-minimal.
- tests/unit-core tests are also linked to
  jerry-port-default-minimal.
Tools adapted.
- build.py has --jerry-port-default instead of --port-dir.
- check-*.sh have paths updated (jerry-port/default instead
  of targets/default).
Miscellaneous.
- Dropped #ifndefs from jerryscript-port.h. It is a public
  header of the jerry-core library, which means that it must
  not contain configuration-dependent parts (once the library is
  built with some config macros and the archive and the headers
  are installed, there is no way for the header to tell what
  those config macrose were).
- Added documentation comments to the JobQueue Port API (in
  jerryscript-port.h) and to several default port
  implementation functions (in jerry-port/default).

JerryScript-DCO-1.0-Signed-off-by: Akos Kiss [email protected]

martijnthe · 2017-04-24T11:14:57Z

jerry-core/jerryscript-port.h

@@ -140,16 +140,31 @@ bool jerry_port_get_time_zone (jerry_time_zone_t *tz_p);
 */
 double jerry_port_get_current_time (void);

-#ifndef CONFIG_DISABLE_ES2015_PROMISE_BUILTIN
-
-#define JERRY_PORT_ENABLE_JOBQUEUE


I think it makes sense to keep a separate JERRY_PORT_ENABLE_JOBQUEUE.
Libraries could require the job queue, but a project using such library may not want to include Promise.

@martijnthe Do I understand correctly that you are against this change? (Sorry, non-native speaker.) If so, my justification(s):

jerryscript-port.h is a public header. Assume that jerry-core is distributed in binary form, then all you have is the libjerry-core.a binary archive and this header. Then, when you include that header, you have no information on how the lib was configured and built, whether it actually contains ES2015 implementations or not. So, adding config conditionals (#ifndef CONFIG_DISABLE_ES2015_PROMISE_BUILTIN) to a public header is not a good idea. But if you don't have that conditional, then JERRY_PORT_ENABLE_JOBQUEUE is always defined, which then adds no information.

Even if a port implements the job queue but nothing references the implementing functions, then the linker will not link the implementing object into the project.

(Or do I get soething wrong?)

Gotcha! ignore me.

akosthekiss · 2017-04-24T13:04:03Z

@jiangzidong As it currently stands, I get assertion failures with jerryscript/tests/jerry-test-suite/es2015/25/25.04/25.04.04/25.04.04-00{2,3,4}.js (that's Promises) in ecma_free_string_list (that's literal storage finalization during the cleanup phase). I managed to figure out that "a" (with 002.js and 003.js) and "b" (with 004.js) string literals have a refcount of 2. Do you have any idea what might cause the problem?

akosthekiss · 2017-04-24T19:01:39Z

@jiangzidong Ah, found it. It was my fault, the jobqueue_run was not executed at all, so enqueued items didn't get released.

Note to self: next time, rewrite #ifdef ENABLE to #ifndef DISABLE, not to #ifdef DISABLE; that single 'n' really does matter...

LaszloLango · 2017-04-25T06:41:33Z

tools/build.py

    build_options.append('-DJERRY_LIBC=%s' % arguments.jerry_libc)
    build_options.append('-DJERRY_LIBM=%s' % arguments.jerry_libm)
    build_options.append('-DFEATURE_JS_PARSER=%s' % arguments.js_parser)
    build_options.append('-DEXTERNAL_LINK_LIBS=' + ' '.join(arguments.link_lib))
    build_options.append('-DEXTERNAL_LINKER_FLAGS=' + ' '.join(arguments.linker_flag))
    build_options.append('-DENABLE_LTO=%s' % arguments.lto)
    build_options.append('-DMEM_HEAP_SIZE_KB=%d' % arguments.mem_heap)
-    build_options.append('-DPORT_DIR=%s' % arguments.port_dir)


I don't see how can someone build a custom port if you remove this. What happens when JERRY_PORT_DEFAULT is off?

If I get it right, currently,

curie_bsp, mbedos5, and nuttx-stm32f4 use their own completely cmake-free build systems and all their port implementation is in their own subtree -- so, for them, it is completely irrelevant whether the default port is built or not,

mbed and zephyr actually use cmake but do not specify PORT_DIR and so build the default port into jerry-core, but also implement all port functions in their own sources, which means that they will be used instead of the lib-provided code, so the default port code in libjerry-core is superfluous,

particle, riot-stm32f4, and esp8266 do rely on the default port.

So, for in-tree targets and for now, it's this third category only that needs to be updated with one more lib to link (I've clearly missed that).

For external targets, they are free to decide. If they want to use the default port, they will have to specify that as a link dependency, just like particle, riot-stm32f4, and esp8266 will have to do it (that's a change, true). If they want custom port implementation, they can do as the other in-tree targets do and hardcode the port functions into their application, or they can separate it out into a different lib. (But that's really up to their build system.)

As for the future of in-tree targets, it's still a bit unclear, but I see the possibility of jerry-port/nuttx-stm32f4 appearing soon. Most probably it will be "just a directory" and not a proper static library with CMakeLists (as the nuttx-stm32f4 does not use cmake, see above).

I hope all the above is not too fuzzy.

TL;DR: in general, if someone wants a custom port, the nicest way is to create a static library like in jerry-port/default and link jerry-core and that library together to the app.

I think I missed the purpose of this refactoring. I though we would like to provide better porting experience with separation of port and target application, but you only moved out the default port and don't give any support for custom ports. IMHO the custom/unique part of a "target" must be the application, so it is reasonable to use their own cmake systems. But jerry-port is mandatory (default or not). I think it so close to the core that we should give better support and not just saying build however you want and link it together.

I think I disagree. I'm not sure, but I think :) Existing examples show that jerry-port -- i.e., separating out the platform-specific code from the app -- is actually not a must. But it is nice for reusability. And if someone (a port/target maintainer) feels the need for the separation, jerry-port/default will show an example how to build a proper static library for a port. In the future, jerry-port/nuttx-stm32f4 may show another way of separation: separated from the app but not turned into a lib as the platform does not work with libs.

zherczeg

LGTM

LaszloLango

LGTM

akosthekiss · 2017-04-25T09:35:55Z

2 LGTMs here. But before proceeding with merging: shall I speculatively try and update the 3 ports (particle, riot-stm32f4, and esp8266) that depend on the default port?

yichoi · 2017-04-25T09:43:24Z

Let's merge first then fix upcoming port issues.

akosthekiss · 2017-04-25T16:55:53Z

OK, I'll merge this now. I've taken a quick look at the above mentioned 3 targets but eventually didn't dare to touch their build system. Some of them (esp8266, at least) seem to be so old and unmaintained that they are/it is already in a broken state. Patching such build system(s) with the default port would not make much sense - they may need additional attention, too.

Request for Maintenance: maintainers of targets, especially of particle, riot-stm32f4, and esp8266, please, take a look at the build systems and follow up on this change as needed.

CC: @galpeter, @janjongboom, @knightburton, @LaszloLango, @pfalcon, @polaroi8d, @poussa, @robertsipka
(The list is a bit arbitrary perhaps, I tried to notify everyone who seems to have worked on the build systems and is still active on github. Sorry in advance if I've left out anybody, was not intentional.)

This commit changes the concept of JerryScript port implementations from a simple directory of C source files (which get injected among the sources of `jerry-core`) into a proper static library (which may be linked to an application together with `jerry-core`). As a consequence, this commit introduces a new library to the JerryScript component architecture: the sources of the default port implementation form `jerry-port-default`. Changes in more detail: - The sources in `targets/default` are moved to `jerry-port/default` and are turned into a proper static library. - Actually, the default port implementation has two library variants, one that implements the bare minimum only (`jerry-port-default-minimal`) and one that has some extra functionalities specific to this implementation (the "full" `jerry-port-default`). - The new libraries have an interface header in `jerry-port/default/include`, which extends the common `jerryscript-port.h` API with functions specific to these libraries. - All non-standard port functions have now the `jerry_port_default_` prefix (this affects `jobqueue_init` and `jobqueue_run`). - The jobqueue implementation functions became config macro independent: it is now the responsibility of the linker to pick the needed objects from the library, and omit those (e.g., jobqueue-related code) that are not referenced. - Build of the libraries can be controlled with the new `JERRY_PORT_DEFAULT` cmake option. - The cmake option `PORT_DIR` is dropped, and `PORT_DIR/*.c` is not appended to `jerry-core` sources. - Instead, the `jerry` tool of `jerry-main` links to `jerry-port-default`, while `jerry-minimal` links to `jerry-port-default-minimal`. - `tests/unit-core` tests are also linked to `jerry-port-default-minimal`. - Tools adapted. - `build.py` has `--jerry-port-default` instead of `--port-dir`. - `check-*.sh` have paths updated (`jerry-port/default` instead of `targets/default`). - Miscellaneous. - Dropped `#ifndef`s from `jerryscript-port.h`. It is a public header of the `jerry-core` library, which means that it must not contain configuration-dependent parts (once the library is built with some config macros and the archive and the headers are installed, there is no way for the header to tell what those config macrose were). - Added documentation comments to the JobQueue Port API (in `jerryscript-port.h`) and to several default port implementation functions (in `jerry-port/default`). JerryScript-DCO-1.0-Signed-off-by: Akos Kiss [email protected]

glistening · 2017-04-26T02:29:27Z

@akosthekiss I am working for tizenrt-artik05x port that I pushed PR before this PR. I removed as this breaking change is landed. For your information, tizenrt-artik05x cannot use jerry-default-port as it is since tizenrt does not provide vfprintf that used by jerry_port_console and jerry_port_log in default-io.c. Perhaps, I will copy almost all code from default port and change some part. Also I will investigate current default jobqueue whether it can be used in NuttX and tizenrt.

akosthekiss added the jerry-port Related to the port API or the default port implementation label Apr 24, 2017

martijnthe reviewed Apr 24, 2017

View reviewed changes

akosthekiss force-pushed the port-dir-to-lib branch from a68af4a to 47e6aa8 Compare April 24, 2017 18:33

akosthekiss changed the title ~~WIP: Turn port implementations into proper libraries~~ Turn port implementations into proper libraries Apr 24, 2017

LaszloLango reviewed Apr 25, 2017

View reviewed changes

LaszloLango mentioned this pull request Apr 25, 2017

target: tizenrt-artik05x: Initial porting #1775

Merged

zherczeg approved these changes Apr 25, 2017

View reviewed changes

LaszloLango approved these changes Apr 25, 2017

View reviewed changes

akosthekiss force-pushed the port-dir-to-lib branch from 47e6aa8 to 7889637 Compare April 25, 2017 15:26

akosthekiss force-pushed the port-dir-to-lib branch from 7889637 to 54e540a Compare April 25, 2017 16:57

akosthekiss merged commit fdbbd0e into jerryscript-project:master Apr 25, 2017

akosthekiss deleted the port-dir-to-lib branch April 25, 2017 17:33

akosthekiss mentioned this pull request Apr 25, 2017

[Discussion] Split port implementation out of applications (and drop PORT_DIR) #1776

Closed

glistening mentioned this pull request Apr 26, 2017

target: tizenrt-artik05x: Add missing jerry_port.c #1786

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Turn port implementations into proper libraries #1777

Turn port implementations into proper libraries #1777

Uh oh!

akosthekiss commented Apr 24, 2017

Uh oh!

martijnthe Apr 24, 2017 •

edited

Loading

Uh oh!

akosthekiss Apr 24, 2017

Uh oh!

martijnthe Apr 24, 2017

Uh oh!

akosthekiss commented Apr 24, 2017

Uh oh!

akosthekiss commented Apr 24, 2017

Uh oh!

LaszloLango Apr 25, 2017

Uh oh!

akosthekiss Apr 25, 2017

Uh oh!

LaszloLango Apr 25, 2017

Uh oh!

akosthekiss Apr 25, 2017

Uh oh!

zherczeg left a comment

Uh oh!

LaszloLango left a comment

Uh oh!

akosthekiss commented Apr 25, 2017

Uh oh!

yichoi commented Apr 25, 2017

Uh oh!

akosthekiss commented Apr 25, 2017 •

edited

Loading

Uh oh!

glistening commented Apr 26, 2017 •

edited

Loading

Uh oh!

Uh oh!

Turn port implementations into proper libraries #1777

Turn port implementations into proper libraries #1777

Uh oh!

Conversation

akosthekiss commented Apr 24, 2017

Uh oh!

martijnthe Apr 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akosthekiss Apr 24, 2017

Choose a reason for hiding this comment

Uh oh!

martijnthe Apr 24, 2017

Choose a reason for hiding this comment

Uh oh!

akosthekiss commented Apr 24, 2017

Uh oh!

akosthekiss commented Apr 24, 2017

Uh oh!

LaszloLango Apr 25, 2017

Choose a reason for hiding this comment

Uh oh!

akosthekiss Apr 25, 2017

Choose a reason for hiding this comment

Uh oh!

LaszloLango Apr 25, 2017

Choose a reason for hiding this comment

Uh oh!

akosthekiss Apr 25, 2017

Choose a reason for hiding this comment

Uh oh!

zherczeg left a comment

Choose a reason for hiding this comment

Uh oh!

LaszloLango left a comment

Choose a reason for hiding this comment

Uh oh!

akosthekiss commented Apr 25, 2017

Uh oh!

yichoi commented Apr 25, 2017

Uh oh!

akosthekiss commented Apr 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glistening commented Apr 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

martijnthe Apr 24, 2017 •

edited

Loading

akosthekiss commented Apr 25, 2017 •

edited

Loading

glistening commented Apr 26, 2017 •

edited

Loading