-
Notifications
You must be signed in to change notification settings - Fork 686
Turn port implementations into proper libraries #1777
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
Turn port implementations into proper libraries #1777
Conversation
|
|
||
| #ifndef CONFIG_DISABLE_ES2015_PROMISE_BUILTIN | ||
|
|
||
| #define JERRY_PORT_ENABLE_JOBQUEUE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@martijnthe Do I understand correctly that you are against this change? (Sorry, non-native speaker.) If so, my justification(s):
jerryscript-port.his a public header. Assume thatjerry-coreis distributed in binary form, then all you have is thelibjerry-core.abinary 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, thenJERRY_PORT_ENABLE_JOBQUEUEis 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?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha! ignore me.
|
@jiangzidong As it currently stands, I get assertion failures with |
a68af4a to
47e6aa8
Compare
|
@jiangzidong Ah, found it. It was my fault, the Note to self: next time, rewrite |
| 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how can someone build a custom port if you remove this. What happens when JERRY_PORT_DEFAULT is off?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
LaszloLango
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
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? |
|
Let's merge first then fix upcoming port issues. |
47e6aa8 to
7889637
Compare
|
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 |
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 akiss@inf.u-szeged.hu
7889637 to
54e540a
Compare
|
@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, |
This is the first implementation for #1776 to showcase the idea presented there.
Note that this is a breaking change!
PORT_DIRoption, it will not work anymore.linked to applications that link to jerry.
However, most probably it will not break most uses cases.
PORT_DIRcmake option.so there will be no need to link anything else but
jerry-core, just like before.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 (whichmay be linked to an application together with
jerry-core). As aconsequence, 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/defaultare moved tojerry-port/defaultand are turned into a proper static library.
variants, one that implements the bare minimum only
(
jerry-port-default-minimal) and one that has some extrafunctionalities specific to this implementation (the "full"
jerry-port-default).jerry-port/default/include, which extends the commonjerryscript-port.hAPI with functions specific to theselibraries.
jerry_port_default_prefix (this affectsjobqueue_initandjobqueue_run).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.
JERRY_PORT_DEFAULTcmake option.The cmake option
PORT_DIRis dropped, andPORT_DIR/*.cis notappended to
jerry-coresources.jerrytool ofjerry-mainlinks tojerry-port-default, whilejerry-minimallinks tojerry-port-default-minimal.tests/unit-coretests are also linked tojerry-port-default-minimal.Tools adapted.
build.pyhas--jerry-port-defaultinstead of--port-dir.check-*.shhave paths updated (jerry-port/defaultinsteadof
targets/default).Miscellaneous.
#ifndefs fromjerryscript-port.h. It is a publicheader of the
jerry-corelibrary, which means that it mustnot 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).
jerryscript-port.h) and to several default portimplementation functions (in
jerry-port/default).JerryScript-DCO-1.0-Signed-off-by: Akos Kiss akiss@inf.u-szeged.hu