Skip to content

CMake: Rename TEST_TARGET to APP_TARGET in greentea test CMakeLists.txt file #14258

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed

Conversation

rajkan01
Copy link
Contributor

@rajkan01 rajkan01 commented Feb 9, 2021

Summary of changes

  • mbed_generate_options_for_linker is now always tied up with APP_TARGET CMake variable so renamed TEST_TARGET to APP_TARGET in all greentea test CMakeLists.txt

Impact of changes

None.

Migration actions required

None.

Documentation

None.


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

[] 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


- mbed_generate_options_for_linker is now always tied up with APP_TARGET CMake variable
  So renamed TEST_TARGET to APP_TARGET in all greentea test CMakeLists.txt
@ciarmcom ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Feb 9, 2021
@ciarmcom ciarmcom requested review from 0xc0170, hugueskamba and a team February 9, 2021 20:30
@ciarmcom
Copy link
Member

ciarmcom commented Feb 9, 2021

@rajkan01, thank you for your changes.
@hugueskamba @0xc0170 @ARMmbed/mbed-os-security @ARMmbed/mbed-os-connectivity @ARMmbed/mbed-os-core @ARMmbed/mbed-os-hal @ARMmbed/mbed-os-maintainers please review.

@mergify mergify bot added needs: CI and removed needs: review labels Feb 10, 2021
@0xc0170
Copy link
Contributor

0xc0170 commented Feb 10, 2021

@rwalton-arm I've noticed this as well as I was testing older example from a user. APP_TARGET is required for an app to define now as we removed the function call to get app name for linker scripts. Is this intended behavior?

We got mbed_configure_app_target that app is calling, either we remove that one and just use APP_TARGET. Or leave there this app target, and get the name of app target we can attach to. Or any other suggestions, this is the TODO:

# TODO: Remove this and find a more idiomatic way of passing compile definitions to CPP without
# using global properties.
mbed_generate_options_for_linker(${APP_TARGET} LINKER_PREPROCESS_DEFINITIONS)
set_property(GLOBAL PROPERTY COMPILE_DEFS_RESPONSE_FILE ${LINKER_PREPROCESS_DEFINITIONS})

@rajkan01 This needs to be addressed, there is inconsistency in our CMake now.

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.

Hold on, let's review why is this required and if it possible to fix. This is related to our story with object libraries.

As this broke the test, we need to fix as soon as possible. The object libs will take much longer to get the changes in.

I'll look at addressing this issue now and continue with object libraries the next sprint.

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

0xc0170 commented Feb 10, 2021

This should address the issue and this pull request should not be needed - please test #14265

@rajkan01
Copy link
Contributor Author

This should address the issue and this pull request should not be needed - please test #14265
@0xc0170 With PR #14265 changes (removing dependency with APP_TARGET CMake variable) solves the issue of renaming TEST_TARET to APP_TARGET but failed to create map file for GCC_ARM toolchain with DISCO_L475VG_IOT01A

@rajkan01 rajkan01 closed this Feb 10, 2021
@mergify mergify bot removed needs: work release-type: patch Indentifies a PR as containing just a patch labels Feb 10, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants