Move os module under the devices. #28
Conversation
|
I couldn't try out on artik053 device. |
| build_flags.append('--target-board=artik05x') | ||
| build_flags.append('--sysroot=%s' % paths.TIZENRT_OS_PATH) | ||
|
|
||
| else: | ||
| console.fail('Non-minimal IoT.js build failed, unsupported ' | ||
| 'device-os (%s-%s) combination!' % | ||
| (self.device.get_type(), self.os)) | ||
| 'device (%s)!' % device.get_type()) |
There was a problem hiding this comment.
That was created to throw an error when a not supported device - os combination was defined by the user (e.g. rpi2 - nuttx). Since this patch hides the os selection from the user, we can remove the else case fully.
There was a problem hiding this comment.
I think we should keep this case. It is a remainder for us which ensures to set the appropriate build flags when adding a new device.
|
|
||
| # prebuild the OS | ||
| os = device.get_os() | ||
| os.prebuild(self) |
There was a problem hiding this comment.
Are you sure that we need to prebuild the os in the app's build method? (I think they are two different things, so the app should not build the os in order to be they independent.)
I think it would be clearer if the os.prebuild could happen in the device's __init__ method (after the os instantiation), because the os is the part of the device right now.
There was a problem hiding this comment.
There are some function calls in the os.prebuild method which depends on the application. This is a bigger change compared to the previous state where these functions were called from the os module's __init__ method. That solution was depend on both of the application and the device (which I think is worse).
But I'am agree with you, I will thinking on a proper solution to eliminate these dependencies.
|
|
||
| # Run the buildscript. | ||
| utils.execute(paths.IOTJS_PATH, 'tools/build.py', build_flags) | ||
|
|
||
| os.build(self, self.buildtype, 'all') |
There was a problem hiding this comment.
Maybe this can be moved to the device.flash method to be the app and the os builds independent.
There was a problem hiding this comment.
It would not be a right approach to call the os.build function in the device.flash. It must be independent from that. It seems that there are still some unresolved dependencies. We should talk about it offline and implements a better solution.
|
|
||
| # prebuild the OS | ||
| os = device.get_os() | ||
| os.prebuild(self) |
There was a problem hiding this comment.
The same as in case of iotjs. (above)
| build_flags = [ | ||
| '-f', | ||
| 'targets/tizenrt-artik053/Makefile.tizenrt', | ||
| 'LIBTARGET_DIR=' + paths.TIZENRT_BUILD_LIBRARIES_PATH | ||
| ] | ||
| utils.execute(paths.JERRY_PATH, 'make', build_flags) | ||
|
|
||
| def skip_test(self, test): | ||
| os.build(self, self.buildtype, 'all') |
There was a problem hiding this comment.
The same as in case of iotjs. (above)
|
@robertsipka There will be a change in the artik053 build. You don't have to worry about it. I will update it later. |
|
@robertsipka rebase please |
|
LGTM after rebasing the commit. The dependency elimination (commented above) could be an other independent PR. |
It is unnecessary to supports several OS for each device. We should support only NuttX-stm32f4, Linux-rpi2, TizenRT-artik053, etc. pairs. JSRemoteTest-DCO-1.0-Signed-off-by: Robert Sipka rsipka.uszeged@partner.samsung.com
robertsipka
force-pushed
the
refactor_os_device_pairs
branch
from
f5b3479
to
435f5a9
Compare
|
I've rebased it with master. |
|
LGTM |
It is unnecessary to supports several OS for each device.
We should support only NuttX-stm32f4, Linux-rpi2, TizenRT-artik053, etc. pairs.
JSRemoteTest-DCO-1.0-Signed-off-by: Robert Sipka rsipka.uszeged@partner.samsung.com