Skip to content

Fix application profile link mess #410

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

Merged

Conversation

ccli8
Copy link

@ccli8 ccli8 commented Dec 30, 2024

Summary of changes

This PR tries to address #407, including:

  1. Provide one approach to address cmake link mess with mbed-core-flags/
    mbed-rtos-flags whose links or not depends on selected application profile,
    full or bare-metal.
    1. Add target config option application-profile for application to
      select which application profile to use. Applications are responsible
      for make this target config option consitent with link selection of
      mbed-os/mbed-baremetal.
    2. Add cmake interface library mbed-application-profile-flags. It is to
      substitute for mbed-core-flags/mbed-rtos-flags. Library camke targets
      should change to link this one to build upon correct application profile
      selection.
  2. All in-tree library targets link mbed-application-profile-flags instead of mbed-core-flags/mbed-rtos-flags to build upon correct application profile.

Impact of changes

To link mbed-baremetal, target.application-profile must be set to bare-metal in mbed_app.json5.


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)

@ccli8 ccli8 force-pushed the build_fix_app_profile_link_mess branch from 1f33f26 to 219c536 Compare December 30, 2024 10:09
@ccli8 ccli8 force-pushed the build_fix_app_profile_link_mess branch 2 times, most recently from 5feb822 to 1c8f00e Compare December 31, 2024 06:42
@multiplemonomials
Copy link
Collaborator

multiplemonomials commented Jan 1, 2025

Woah, this is a lot to take on! Thank you for working on this!

Haven't looked at the code yet but just going on the description:

  • Could we rename mbed-application-profile-flags to either mbed-os-flags or mbed-flags? Going to be typing that name 100000 times so I'd like it a bit shorter, and that doesn't lose information.
  • We will need to provide an alias so that libraries and applications that use mbed-core-flags will still work. Luckily CMake makes that easy to do with add_library(ALIAS)

@ccli8 ccli8 force-pushed the build_fix_app_profile_link_mess branch from 1c8f00e to 3301815 Compare January 2, 2025 01:30
@ccli8
Copy link
Author

ccli8 commented Jan 2, 2025

Could we rename mbed-application-profile-flags to either mbed-os-flags or mbed-flags? Going to be typing that name 100000 times so I'd like it a bit shorter, and that doesn't lose information.

That's fine. Rename mbed-application-profile-flags to mbed-os-flags.

We will need to provide an alias so that libraries and applications that use mbed-core-flags will still work. Luckily CMake makes that easy to do with add_library(ALIAS)

There is compatible concern. My original idea is:

  • new mbed-core-flags = old mbed-core-flags
  • new mbed-rtos-flags = old mbed-rtos-flags
  • added mbed-os-flags = old mbed-core-flags + old mbed-rtos-flags if target.application-profile isfull.

To achieve above compatible, updated idea would become:

  • new mbed-core-flags = old mbed-core-flags + old mbed-rtos-flags if target.application-profile isfull.
  • new mbed-rtos-flags = old mbed-rtos-flags
  • added mbed-os-flags = new mbed-core-flags or become unnecessary

That is, mbed-core-flags is overloaded dependent on target.application-profile. And only one of mbed-os and mbed-baremetal target would be created dependent on target.application-profile, but not both. For example, when target.application-profile is full, mbed-baremetal cannot link mbed-core-flags because mbed-core-flags also links mbed-rtos-flags.
.

@multiplemonomials
Copy link
Collaborator

multiplemonomials commented Jan 2, 2025

Makes sense to me! I definitely think that, depending on the application profile setting, we should make only the mbed-baremetal or mbed-os cmake targets available.

@ccli8 ccli8 force-pushed the build_fix_app_profile_link_mess branch from 3301815 to b8fa716 Compare January 3, 2025 03:14
@ccli8
Copy link
Author

ccli8 commented Jan 3, 2025

Update as below:

  1. Drop mbed-application-profile-flags (or its renamed versions mbed-os-flags). It is replaced with overloaded mbed-core-flags.
  2. Overload mbed-core-flags/mbed-core-sources so that they will involve mbed-rtos-flags/mbed-rtos-sources or not, depending on target.application-profile setting.
  3. All libraries must link mbed-core-flags and no mbed-rtos-flags, or the link mess issue remains.
  4. Both mbed-os and mbed-baremetal are still created, but one will become invalid and get caught on application link, depending on target.application-profile setting.

@JohnK1987
Copy link
Member

In targets.json is also this parameter

"supported_application_profiles": [
"full"

I'm not sure if this has an use case at this moment.

@ccli8 ccli8 force-pushed the build_fix_app_profile_link_mess branch from 56d3a96 to f164549 Compare January 6, 2025 02:19
@ccli8
Copy link
Author

ccli8 commented Jan 6, 2025

In targets.json is also this parameter supported_application_profiles

The added application-profile config option and its supported options full and bare-metal just follows the naming of supported_application_profiles.

@ccli8 ccli8 force-pushed the build_fix_app_profile_link_mess branch from a658bfc to 08624c0 Compare January 6, 2025 09:38
ccli8 added 6 commits January 10, 2025 09:41
This provides one approach to address cmake link mess with
mbed-core-flags/mbed-rtos-flags whose links or not depends on selected
application profile, full or bare-metal.
1.  Add target config option application-profile for application to
    select which application profile to use. Applications are responsible
    for making this target config option consitent with link selection of
    mbed-os/mbed-baremetal.
2.  Overload mbed-core-flags/mbed-core-sources so that they can involve
    mbed-rtos-flags/mbed-rtos-sources or not depending on
    target.application-profile setting
…remetal

This guards from mismatch of target.application-profile and link
libraries mbed-os/mbed-baremetal.
With mbed-core-flags overloaded to match target.application-profile setting,
all libraries should link mbed-core-flags instead of mbed-rtos-flags so that
they build upon correct application profile, involving mbed-rtos-flags or not.
With mbed-core-flags/mbed-core-sources being overloaded to match
target.application-profile setting, mbed-os should link only
mbed-core-flags/mbed-core-sources which will involve mbed-rtos-flags/
mbed-rtos-sources as config.
MBED_GREENTEA_TEST_BAREMETAL can become deprecated and be removed. The
'auto' option of target.application-profile needn't respect it.
MBED_GREENTEA_TEST_BAREMETAL can become deprecated and be removed.
Respect target.application-profile setting instead to resolve link
library.
@ccli8 ccli8 force-pushed the build_fix_app_profile_link_mess branch from 08624c0 to c35976f Compare January 10, 2025 02:29
Copy link
Collaborator

@multiplemonomials multiplemonomials left a comment

Choose a reason for hiding this comment

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

Thank you for making this fix!

@multiplemonomials multiplemonomials merged commit a468f27 into mbed-ce:master Jan 18, 2025
52 checks passed
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.

None yet

3 participants