Mbed-os: pal_crypto.h file name conflict with Pelion client

[adamelhakham]

Description

There is a new file in the BLE stack called pal_crypto.h:
https://github.com/ARMmbed/mbed-os/blob/master/features/FEATURE_BLE/targets/TARGET_CORDIO/stack/platform/include/pal_crypto.h

• The problem is that the Pelion client has a porting layer called PAL and one of the files is called pal_Crypto.h. This causes build failures when building on a Windows machine (file system not case sensitive).
  It would be great if that file name can be changed, because Pelion Client should work out of the box.

• Another issue: When I try to remove the BLE feature (for the UBLOX_EVK_ODIN_W2 target) with the following line: "target.features_remove" : ["BLE"]
  The build fails for ARMCC 6. This seems to be another bug (unless I am not removing the BLE feature properly)
  

Issue request type

[ ] Question
[ ] Enhancement
[x ] Bug

[ciarmcom]

Internal Jira reference: https://jira.arm.com/browse/MBOCUSTRIA-977

[0xc0170]

@ARMmbed/mbed-os-pan Can you please review

[paul-szczepanek-arm]

The file belongs to an external lib - cordio bluetooth stack, I think the real solution is to fix the build system to require full include paths for includes that are not part of the feature being compiled.

[pan-]

To build on @paul-szczepanek-arm answer, the build system is the real offender here, we can add all the workaround in the world, this type of issue will continue to happen whenever a third party library is imported. As long as the build system continue to consider every header to be public there will be collision in common use cases (@geky @ARMmbed/mbed-os-tools ).

Current solution within the os is to fully qualify include paths; of course that doesn't prevent third party library to generate conflicts. As a workaround, I suggest we qualify paths for pal_crypto.h in the Cordio library and engage with the cordio team for a more sustainable solution. In the meantime @adamelhakham, could you make sure that include paths in Pelion client are properly qualified ?

[theotherjimmy]

fix the build system to require full include paths for includes that are not part of the feature being compiled.

That would break _so many_ current projects, it's sort of funny to think about the developer backlash.

[donatieng]

That would break so many current projects, it's sort of funny to think about the developer backlash.

@theotherjimmy Yes - but the longer we wait before fixing this, the bigger the backlash will be when we can't afford not to do it anymore. And nothing would prevent us from leaving that "feature" enabled in user code by default, while starting doing things properly within the OS codebase.

[theotherjimmy]

@donatieng

And nothing would prevent us from leaving that "feature" enabled in user code by default, while starting doing things properly within the OS codebase.

Sounds like it needs to be configurable per-library, or something along those lines.

[bridadan]

I was going to create an issue to brainstorm some ideas about this. Seems like we need _some_ kind of plan going forward. Having (almost) every directory being included as an include path already creates other problems with command line length.

[pan-]

@theotherjimmy Yes; I think some work can be done with the _conditional build system_ . If library writers can define which path is visible to library consumer then the build system can make sure that headers internal to the library are not exposed to unrelated modules and public headers are not exposed to library that do not consume then.

[theotherjimmy]

@geky FYI

@pan- Great idea! I was thinking the same thing myself.

[0xc0170]

Besides good discussion around how to fix this properly, how to address this for 5.12.0-rc2/3 ? we got short time window. As cordio is external library, we might seek a fix at the pelion if this can be fixed there.

@adamelhakham, could you make sure that include paths in Pelion client are properly qualified ?

@adamelhakham we haven't yet received how this conflicting header file is included.

cc @jenia81

[adamelhakham]

@0xc0170 @pan- not sure what you mean by include path, I use mbed-cli which includes all the features included in the jsons and basically pretty much everything except for tests.
In our case, since targets.json adds the BLE feature for the UBLOX_EVK_ODIN_W2 target, this new pal_crypto.h is included by some source file in the BLE stack. However since we there is also our (Pelion Client) file called pal_Crypto,h and I used a windows machine to build the code the file that actually got included to the BLE source code is our pal_Crypto.h instead of pal_crypto.h of the BLE stack, thus failing the build.

[0xc0170]

@adamelhakham where is this file included? Want to check the sources. our initial assumption is there will be "#include "pal_crypto.h". If you can share the repository (best would be the files directly including it in Pelion code base).

[adamelhakham]

• we include pal_Crypto.h here:
  https://github.com/ARMmbed/mbed-cloud-client-sources-internal/blob/master/mbed-client-pal/Source/PAL-Impl/Services-API/pal.h
• BLE stack includes pal_crypto.h the file here:
  https://github.com/ARMmbed/mbed-os/blob/master/features/FEATURE_BLE/targets/TARGET_CORDIO/stack/ble-host/sources/sec/common/sec_main.h

[0xc0170]

As the first one is our file, I would suggest to update the line 39 to #include Services-API/pal_Crypto.h". This would be workaround - will enable you to continue now and us to work towards the better fix

Update: or us in cordio as below

[donatieng]

@0xc0170 in the short term our team can issue a workaround (by amending this line: https://github.com/ARMmbed/mbed-os/blob/83d70199d1d0c4774327110f524e2386dad730b2/features/FEATURE_BLE/targets/TARGET_CORDIO/stack/ble-host/sources/sec/common/sec_main.h#L26) and we'll need the Client team to do the same. Once that's in I suggest reducing the priority of the issue, but leaving it open to track the underlying build system issue.

[0xc0170]

@donatieng +1, the same answer at almost the same time :)

[adbridge]

@donatieng Will you be landing a PR asap ?

[adbridge]

@JanneKiiskila @TeemuKultala Who needs to do this on the client side ?

[paul-szczepanek-arm]

I can do cordio

[paul-szczepanek-arm]

https://github.com/ARMmbed/mbed-os/pull/10042

[yogpan01]

@adamelhakham Can you do the proposed fix on mbed-client-pal and check if that resolves the issue?

[0xc0170]

@adamelhakham Can you do the proposed fix on mbed-client-pal and check if that resolves the issue?

Any update? Cordio fix is now going in.

[0xc0170]

10042 merged. We are waiting for PAL fix (reopening this issue until fixed)

[adamelhakham]

@adamelhakham Can you do the proposed fix on mbed-client-pal and check if that resolves the issue?

Yes I am checking, will keep you posted

[adamelhakham]

@adamelhakham Can you do the proposed fix on mbed-client-pal and check if that resolves the issue?

Yes I am checking, will keep you posted

@yogpan01 @0xc0170 OK this indeed fixes the issue, I am preparing the relevant PRs for the client

[teetak01]

This ticket can be closed. Any client side changes will not impact OS releasing at this point, because all those changes will be in internal repositories at this point.