Skip to content
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

mbed_create_distro() reborn: a function to make adding multiple targets easy #15126

Merged
merged 4 commits into from
Oct 19, 2021

Conversation

multiplemonomials
Copy link
Contributor

@multiplemonomials multiplemonomials commented Oct 3, 2021

Summary of changes

This PR adds mbed_create_distro(), a function that lets you compile multiple apps in an Mbed OS project without waiting for Mbed OS to build multiple times.

You can use it like this:

mbed_create_distro(mbed_for_my_app mbed-os mbed-storage-kvstore mbed-storage-filesystem)

add_executable(myapp1 MyApp1.cpp)
target_link_libraries(myapp1 PRIVATE mbed_for_my_app)
mbed_set_post_build(myapp1)

add_executable(myapp2 MyApp2.cpp)
target_link_libraries(myapp2 PRIVATE mbed_for_my_app)
mbed_set_post_build(myapp2)

Both myapp1 and myapp2 will act like they were linked to mbed-os, mbed-storage-kvstore, and mbed-storage-filesystem. Note that if you actually did target_link_libraries(myapp1 PRIVATE mbed-os mbed-storage-kvstore mbed-storage-filesystem), it would compile a new version of the Mbed OS source files for each target. However, using mbed_create_distro(), Mbed OS will only be compiled once.

Note that I originally submitted this like 6 months ago, this is an update of the original PR (#14274). At the time I closed that PR because it seemed like Mbed OS was going to switch to using object libraries for all targets, which would also fix the building-multiple-times problem. However, it's been close to 6 months and that change still hasn't happened, and Mbed CLI still isn't usable for RPL until this issue is fixed. So, I'd like to resubmit this PR as at least a stopgap measure until that change happens.

If merged, this will provide a satisfactory fix for #13981.

Impact of changes

mbed_create_distro() is now offered by the build system. This function replaces mbed_configure_app_target() and creates a separate "distribution" OBJECT library from the given Mbed OS libraries that only needs to be compiled once.

Migration actions required

None

Documentation

Docs provided in code comments.


Pull request type

[X] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[X] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers


@multiplemonomials multiplemonomials changed the title Create distro reborn mbed_create_distro() reborn: a function to make adding multiple targets easy Oct 3, 2021
@ciarmcom ciarmcom requested a review from a team October 3, 2021 20:30
@ciarmcom
Copy link
Member

ciarmcom commented Oct 3, 2021

@multiplemonomials, thank you for your changes.
@ARMmbed/mbed-os-maintainers please review.

@ladislas
Copy link
Contributor

ladislas commented Oct 3, 2021

Thanks @multiplemonomials! I'll try it asap :)

@0xc0170 0xc0170 requested a review from a team October 4, 2021 07:58
0xc0170
0xc0170 previously requested changes Oct 4, 2021
Copy link
Contributor

@0xc0170 0xc0170 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for resubmitting this fix, let's get this in.

I only found some debug messages left, otherwise looks fine to me.

endif()
endforeach()

#message("REMAINING_MODULES: ${REMAINING_MODULES}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shall we remove this debug messages (line 63 and 51 as well) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed!

@0xc0170 0xc0170 requested a review from a team October 4, 2021 13:20
@mergify mergify bot dismissed 0xc0170’s stale review October 5, 2021 04:58

Pull request has been modified.

get_property(PROP_IS_DEFINED TARGET ${SOURCE} PROPERTY ${PROPERTY} SET)
if(PROP_IS_DEFINED)
get_property(PROP_VALUE TARGET ${SOURCE} PROPERTY ${PROPERTY})
#message("Copying ${PROPERTY} from ${SOURCE} -> ${DESTINATION}: ${PROP_VALUE} ")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have missed one more

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed!

@ladislas
Copy link
Contributor

ladislas commented Oct 5, 2021

I've tried it with my template project and I can confirm this works!

ladislas/mbed-cmake-template#10

The good news is that it's not mandatory to do it, it won't break people's projects if they don't use the mbed_create_distro() function.

I can't try it now with our big project (https://github.com/leka/LekaOS) as it will need to change a lot of things but I think it's a good start. And we can start building from this PR.

@mergify mergify bot added needs: CI and removed needs: work labels Oct 6, 2021
@0xc0170
Copy link
Contributor

0xc0170 commented Oct 6, 2021

CI started, however waiting for one more approval from the core team

@mbed-ci
Copy link

mbed-ci commented Oct 6, 2021

Jenkins CI Test : ✔️ SUCCESS

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs Status
jenkins-ci/mbed-os-ci_unittests ✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_greentea-test ✔️

@0xc0170 0xc0170 requested a review from a team October 6, 2021 13:13
@0xc0170 0xc0170 added this to In progress (8) in Mbed Core via automation Oct 6, 2021
@ladislas
Copy link
Contributor

ladislas commented Oct 8, 2021

ping :)

@ladislas
Copy link
Contributor

More and more people on the forum are moving to mbed cli2 and are experiencing the issue this PR is resolving in a elegant, easy and opt-in manner.

It would be great if it could be merged so people could start using it and improve if needed.

Mbed Core automation moved this from In progress (8) to Reviewer approved & awaiting CI & merge (3) Oct 19, 2021
@0xc0170 0xc0170 merged commit f69af59 into ARMmbed:master Oct 19, 2021
Mbed Core automation moved this from Reviewer approved & awaiting CI & merge (3) to Done Oct 19, 2021
@mergify mergify bot removed the ready for merge label Oct 19, 2021
@ladislas
Copy link
Contributor

amazing! thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Mbed Core
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

7 participants