Change open-amp/libmetal to build non-recursive #7769

galak · 2018-05-22T23:24:33Z

This fixes issue #7673.

codecov-io · 2018-05-23T00:05:33Z

Codecov Report

Merging #7769 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #7769   +/-   ##
=======================================
  Coverage   55.01%   55.01%           
=======================================
  Files         483      483           
  Lines       53946    53946           
  Branches    10493    10493           
=======================================
  Hits        29678    29678           
  Misses      19982    19982           
  Partials     4286     4286

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 24b0b3c...b2a7971. Read the comment docs.

SebastianBoe · 2018-05-23T08:24:45Z

ext/hal/CMakeLists.txt

@@ -1,5 +1,13 @@
+if(CONFIG_LIBMETAL)


This libmetal-specific code does not belong in ext/hal/CMakeLists.txt.

It should be pushed into libmetal somehow.

How, I don't think it makes sense to modify the CMakeLists.txt file in libmetal. I don't see a way to do this beyond moving libmetal down another dir, but that seems silly just to avoid a little CMake foo in ext/hal that is clearly related to libmetal.

I agree with @SebastianBoe , we should not allow the top level CMakeLists.txt file to be come the place where modules are being configured, this will become unmaintainable if everyone starts putting their logic in there. Going down one level seems reasonable, we did this for a few other modules. Also, the README.libmetal does not belong in there.
All of this seems silly now, but when we move to multi-git structure, it will become necessary.

"Going down one level seems reasonable, we did this for a few other modules."
+1

Ok, I'll change it, what's an example where we do this one additional level stuff?

SebastianBoe · 2018-05-23T08:25:23Z

ext/hal/CMakeLists.txt

 add_subdirectory(atmel)
 add_subdirectory(cmsis)
+add_subdirectory_ifdef(CONFIG_LIBMETAL libmetal)


_if_kconfig(libmetal) is preferable when the define matches the directory name.

SebastianBoe · 2018-05-23T08:40:20Z

ext/lib/ipc/CMakeLists.txt

+  set(WITH_LIBMETAL_FIND OFF CACHE BOOL "" FORCE)
+  set(LIBMETAL_INCLUDE_DIR ${ZEPHYR_BINARY_DIR}/ext/hal/libmetal/lib/include)
+  set(LIBMETAL_LIB ${ZEPHYR_BINARY_DIR}/ext/hal/libmetal/lib)
+endif(CONFIG_OPENAMP)


This should also be moved into an openamp-specific build script.

Same comment as related to libmetal, above.

SebastianBoe · 2018-05-23T10:20:13Z

Shouldn't there be a DNM tag?

It doesn't seem to build ...

Will resume reviewing when it builds and can be tested. Kindly do not dismiss this review
as was done with the other OpenAMP reviews.

EDIT: ninja: error: 'openamp_remote-prefix/src/openamp_remote-build/zephyr/zephyr.bin', needed by 'zephyr/include/generated/core-m0.inc', missing and no known rule to make it

galak · 2018-05-23T11:00:26Z

Shouldn't there be a DNM tag?

It doesn't seem to build ...

Will resume reviewing when it builds and can be tested. Kindly do not dismiss this review
as was done with the other OpenAMP reviews.

EDIT: ninja: error: 'openamp_remote-prefix/src/openamp_remote-build/zephyr/zephyr.bin', needed by 'zephyr/include/generated/core-m0.inc', missing and no known rule to make it

The ninja build issue isn't directly related, there is an issue #7760 for that.

SebastianBoe · 2018-05-23T11:18:37Z

Okay, so it is broken on master as well.

Anyway, I can't review openamp without testing it. So this review is blocked until #7760 is resolved.

galak · 2018-05-23T12:39:02Z

Anyway, I can't review openamp without testing it. So this review is blocked until #7760 is resolved.

Why can't you test it w/Make instead of ninja?

SebastianBoe · 2018-05-23T12:39:48Z

I can. Was not aware #7760 was Ninja-specific.

SebastianBoe · 2018-05-23T13:12:13Z

I can't find any documentation that explains what the files in samples/subsys/ipc/openamp/platform are.

galak · 2018-05-23T15:44:16Z

I can't find any documentation that explains what the files in samples/subsys/ipc/openamp/platform are.

what docs do you expect?

libmetal itself isn't an ipc library, its a generic HAL abstraction library so move it into ext/hal where it belongs. Signed-off-by: Kumar Gala <[email protected]>

Pull in a slightly more recent version of libmetal with changes to the build system to improve integration with Zephyr. This change will break anything that builds with Zephyr and libmetal (which at this point is only the open-amp example). Will fix that shortly. Signed-off-by: Kumar Gala <[email protected]>

With recent changes to libmetal we can now include and build it directly as a zephyr library rather than doing a recursive make. We remove ext/hal/libmetal.cmake as part of this change and introduce a Kconfig option for libmetal. This is a partial fix for issue zephyrproject-rtos#7673. Signed-off-by: Kumar Gala <[email protected]>

Move open-amp to be in ext/lib/ipc/open-amp/open-amp. This allows us to be Zephyr specific files and config like README (for import), CMakeLists.txt file, Kconfig, etc in ext/lib/ipc/open-amp/ that don't conflict with any files that might have the same name in ext/lib/ipc/open-amp/open-amp. Signed-off-by: Kumar Gala <[email protected]>

Pull in a slightly more recent version of open-amp with changes to the build system to improve integration with Zephyr. The recent changes let us embedded open-amp without having to use recursive building. Signed-off-by: Kumar Gala <[email protected]>

With recent changes to open-amp we can now include and build it directly as a zephyr library rather than doing a recursive make. We remove ext/lib/ipc/open-amp.cmake as part of this change and introduce a Kconfig option for open-amp. Fixes: zephyrproject-rtos#7673 Signed-off-by: Kumar Gala <[email protected]>

SebastianBoe · 2018-05-24T07:35:30Z

what docs do you expect?

A toplevel comment in each source file.

SebastianBoe · 2018-05-24T08:43:00Z

Is there any way for a user to know whether OpenAMP is compatible with his platform?

E.g. which platforms do we claim support for the sample? Just lpcxpresso54114_m4? If so that should be explicitly stated in the sample docs instead of implied through the command example and build script code.

The same applies to open-amp itself. Do we claim that open-amp will work on all Zephyr platforms, or just the intersecting platforms of open-amp and Zephyr? This should be explicitly documented in open-amp's documentation.

SebastianBoe · 2018-05-24T12:43:24Z

For application's with self-contained slaves that don't need to import configuration from the master I believe it will be cleaner to have master Kconfig point to an application source directory instead of an image.

As can be seen in this patch:

SebastianBoe@f059efe

It results in less copy-paste boilerplate in the master application build scripts and less magic paths in the prj.conf.

galak · 2018-05-24T13:26:52Z

Is there any way for a user to know whether OpenAMP is compatible with his platform?

E.g. which platforms do we claim support for the sample? Just lpcxpresso54114_m4? If so that should be explicitly stated in the sample docs instead of implied through the command example and build script code.

The same applies to open-amp itself. Do we claim that open-amp will work on all Zephyr platforms, or just the intersecting platforms of open-amp and Zephyr? This should be explicitly documented in open-amp's documentation.

So fair, but is that directly related to these changes?

nashif · 2018-05-24T13:56:13Z

So fair, but is that directly related to these changes?

agree with @galak here, this is about the high priority bug we have lingering, lets resolve any documentation and configuration enhancements to some other PR, it this PR resolve the recursive build issue, lets merge it.

The comments associated with this PR have been addressed, other comments about the sample docs, etc will be addressed in a follow up PR.

galak · 2018-05-24T16:24:48Z

Is there any way for a user to know whether OpenAMP is compatible with his platform?

E.g. which platforms do we claim support for the sample? Just lpcxpresso54114_m4? If so that should be explicitly stated in the sample docs instead of implied through the command example and build script code.

The same applies to open-amp itself. Do we claim that open-amp will work on all Zephyr platforms, or just the intersecting platforms of open-amp and Zephyr? This should be explicitly documented in open-amp's documentation.

I've created PR #7881 for the cleanups you mention in this PR.

SebastianBoe · 2018-05-28T07:43:59Z

So fair, but is that directly related to these changes?

No, but my review of the Open-AMP integration was dismissed so I am doing it now.

galak requested review from dbkinder, nashif and SebastianBoe as code owners May 22, 2018 23:24

galak mentioned this pull request May 22, 2018

Change libmetal to build non-recursively. #7762

Closed

galak removed the request for review from dbkinder May 22, 2018 23:25

galak force-pushed the new-openamp branch from ff76eb9 to 320e5ea Compare May 23, 2018 02:14

SebastianBoe previously requested changes May 23, 2018

View reviewed changes

galak force-pushed the new-openamp branch from 320e5ea to dce3622 Compare May 23, 2018 11:03

galak force-pushed the new-openamp branch from dce3622 to 026edfd Compare May 23, 2018 15:03

galak added 6 commits May 23, 2018 10:44

ext: move libmetal to hal

994a2bc

libmetal itself isn't an ipc library, its a generic HAL abstraction library so move it into ext/hal where it belongs. Signed-off-by: Kumar Gala <[email protected]>

galak force-pushed the new-openamp branch from 026edfd to b2a7971 Compare May 23, 2018 15:46

nashif approved these changes May 24, 2018

View reviewed changes

galak merged commit 4e8438b into zephyrproject-rtos:master May 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change open-amp/libmetal to build non-recursive #7769

Change open-amp/libmetal to build non-recursive #7769

galak commented May 22, 2018

codecov-io commented May 23, 2018 •

edited

Loading

SebastianBoe May 23, 2018

galak May 23, 2018

nashif May 23, 2018

SebastianBoe May 23, 2018

galak May 23, 2018

SebastianBoe May 23, 2018

SebastianBoe May 23, 2018

galak May 23, 2018

SebastianBoe commented May 23, 2018 •

edited

Loading

galak commented May 23, 2018

SebastianBoe commented May 23, 2018

galak commented May 23, 2018

SebastianBoe commented May 23, 2018

SebastianBoe commented May 23, 2018

galak commented May 23, 2018

SebastianBoe commented May 24, 2018

SebastianBoe commented May 24, 2018 •

edited

Loading

SebastianBoe commented May 24, 2018

galak commented May 24, 2018

nashif commented May 24, 2018

galak commented May 24, 2018

SebastianBoe commented May 28, 2018

Change open-amp/libmetal to build non-recursive #7769

Change open-amp/libmetal to build non-recursive #7769

Conversation

galak commented May 22, 2018

codecov-io commented May 23, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebastianBoe commented May 23, 2018 • edited Loading

galak commented May 23, 2018

SebastianBoe commented May 23, 2018

galak commented May 23, 2018

SebastianBoe commented May 23, 2018

SebastianBoe commented May 23, 2018

galak commented May 23, 2018

SebastianBoe commented May 24, 2018

SebastianBoe commented May 24, 2018 • edited Loading

SebastianBoe commented May 24, 2018

galak commented May 24, 2018

nashif commented May 24, 2018

galak commented May 24, 2018

SebastianBoe commented May 28, 2018

codecov-io commented May 23, 2018 •

edited

Loading

SebastianBoe commented May 23, 2018 •

edited

Loading

SebastianBoe commented May 24, 2018 •

edited

Loading