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

Add mbed-create-distro, a function to make adding multiple targets easy #14274

Closed
wants to merge 2 commits into from

Conversation

multiplemonomials
Copy link
Contributor

@multiplemonomials multiplemonomials commented Feb 12, 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. If you actually did that, it would compile a new version of the source files for each target. However, using mbed_create_distro(), Mbed OS will only be compiled once.

If merged, this will provide a satisfactory (though not very clean...) fix for #13981.

Note: I also had to provide a band-aid fix for #14220 in order to make the new code usable. As I discussed here, this works by just assuming that all application targets have the same compile defs in terms of what's needed by the linker script.

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

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

Test results

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

Reviewers

@0xc0170 @hugueskamba


@ciarmcom
Copy link
Member

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

@ladislas
Copy link
Contributor

Thanks @multiplemonomials for the PR, that's great!

I did some initial testing and I confirm it works.

I'll provide a working example asap.

@0xc0170
Copy link
Contributor

0xc0170 commented Feb 12, 2021

If merged, this will provide a satisfactory (though not very clean...) fix for #13981.

Based on your experience, what would you choose from the options we got? would cmake-object-libs branch be cleaner doing this (CMake way) ?

We will allocate time in the upcoming sprint.

@multiplemonomials
Copy link
Contributor Author

I think that cmake-object-libs is a better way but I don't really have the huge amount of time and energy it would require to make the change at this point in development. It's really up to you guys. Whether you're planning to implement it or not, I think it's worth merging this in the interim to provide a workaround for multiple target support.

@mergify mergify bot added the needs: work label Feb 14, 2021
@mergify
Copy link

mergify bot commented Feb 14, 2021

This PR cannot be merged due to conflicts. Please rebase to resolve them.

@mergify mergify bot removed the needs: review label Feb 14, 2021
@0xc0170
Copy link
Contributor

0xc0170 commented Feb 22, 2021

Don't rebase yet. We are evaluating the approaches for the issue. We will get back to this.

@ciarmcom ciarmcom added the stale Stale Pull Request label Feb 25, 2021
@ciarmcom
Copy link
Member

This pull request has automatically been marked as stale because it has had no recent activity. @multiplemonomials, please carry out any necessary work to get the changes merged. Thank you for your contributions.

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 2, 2021

Don't rebase yet. We are evaluating the approaches for the issue. We will get back to this.

I think that cmake-object-libs is a better way but I don't really have the huge amount of time and energy it would require to make the change at this point in development.

Our proposal is to use this object-libs approach. I created initial draft 0xc0170#21. This is a base for refactoring and testing to get entire tree fixed.

I created feature branch here feature-cmake-object-libraries where we will continue fixing libraries (please fetch, test and send pull request against this branch if you find any issues).
Note I'll be rebasing it daily to keep up to date with master. The fix should be completed for upcoming release. If that sounds like a plan, this can be closed and the new PR will be created in couple of days to make it in.

Thank you for providing this to get better understanding of the issue itself and how to resolve it.

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 9, 2021

I'll close this. Please test feature-cmake-object-libraries and let that merge to master asap.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants