search

ARMmbed / mbed-os

Arm Mbed OS is a platform operating system designed for the internet of things
https://mbed.com
Other
4.67k stars 2.98k forks source link

pal_types.h name conflict that breaks any Pelion DM Client with Mbed OS 5.12.0 #10278

Closed screamerbg closed 3 years ago

screamerbg commented 5 years ago

Description

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

It's not hard for see how this doesn't work with Pelion DM Client - https://github.com/ARMmbed/mbed-cloud-client/search?q=pal_types.h&unscoped_q=pal_types.h

This breaks Mbed OS 5.12 for ANY previous version of Pelion DM Client, where a patch to Pelion DM Client won't fix it for customers that don't want to upgrade PDMC.

As a side note - Perhaps a new CI job that checks whether a PR introduces duplicate header files would help with these kind of issues? @OPpuolitaival.

CC @JanneKiiskila

Issue request type

[ ] Question
[ ] Enhancement
[X] Bug
0xc0170 commented 5 years ago

cc @ARMmbed/mbed-os-pan

MarceloSalazar commented 5 years ago

@ARMmbed/mbed-os-pan can you please review and comment?

pan- commented 5 years ago

@MarceloSalazar I'm not sure to understand what is expected from us. Access to the header of the Cordio stack is prefixed in all of our code and it isn't in cloud client code. I'd argue it is a bug in the client code; not mbed-os. We are exposed to this kind of issue whenever new code is added with the build system used.

Monitoring headers file name is a short sighted workaround that doesn't scale outside our code base: If we add a new header file and an existing customer code base already contains it; the customer will face the same issues we are facing now when it upgrades to a new version of the OS or the client.

0xc0170 commented 5 years ago

As suggested above, this is known issue - basically limitation of the build system. The file was added to 5.12.0, this breaks applications - any new file added could, without having proper prefix or unique name.

As this issue is related to the application, can we fix the naming there rather?

MarceloSalazar commented 5 years ago

@0xc0170 this doesn't seem related to the application. One is a feature inside Mbed OS (Cordio), and another is an external library (cloud client).

This kind of issues doesn't happen often. IMO it makes sense to rename one of the files, otherwise, we'll hit this problem with more platforms and applications.

@JanneKiiskila maybe you can rename the one inside cloud-client library?

JanneKiiskila commented 5 years ago

Maybe we should fix the tool chain, this is a symptom of a broken tool chain, really (in my humble opinon). CC: @kjbracey-arm

We'll see what we can do short term, but long term - fix the darn tools.

Is there list of "non-reserved" names? What should it be called instead?

0xc0170 commented 5 years ago

Maybe we should fix the tool chain, this is a symptom of a broken tool chain, really (in my humble opinon). CC: @kjbracey-arm

Definitely, the question is when and how. I believe this issue should be moved to @ARMmbed/mbed-os-tools team (please move if agree and review).

kjbracey commented 5 years ago

The @ARMmbed/mbed-os-core team are doing some folder restructuring with I believe a medium-term view towards scaling back the "all directories on the include path" model introduced in Mbed OS 5.0. This would be related to that work.

As an intermediate tools change, you could maybe do something to modify search order on that "all folder" list - I think at the minute it's alphabetical (which varies depending on case-sensitivity of host OS), which makes unexpected includes pop up quite easily.

Maybe when building C files inside "mbed-os", include folders inside "mbed-os" could be placed earlier in the search order, sort of thing? Rank include folders by "number of path elements in common with current source file" or something?

mark-edgeworth commented 5 years ago

Hmmm, this looks like a conflicting header file name in two separate pieces of code. This is going to occur in any system from time-to-time all the while those header names are not namespaced. It is not a limitation of the build system. This is an issue for mbed OS that the build tools cannot cover for (ignoring hacks of course).

kjbracey commented 5 years ago

It stems from the Mbed OS 5 core design decision to automatically put everything on the path. (For user "simplicity").

Collisions can be reduced by include-site namespacing - getting people to do #include "component/foo.h", as was actually required in Mbed OS 3. That increasingly is happening in Mbed OS core code, but probably not in target or application code.

There is very limited automatic locality logic in the compilers so that #include "foo.h" will search current directory first, but we could maybe improve our situation by extending that by a conscious sort of the include list, so we hit closer files in preference. I think that's no more a "hack" than doing the auto-addition of paths in the first place - if we're going to auto-add paths, we could choose an order based on some better rule than alphabetical.

JanneKiiskila commented 5 years ago

So, should the Cordio code the include "cordio/pal_types.h" instead of "pal_types.h" if that's what components inside Mbed OS do?

kjbracey commented 5 years ago

So, should the Cordio code the include "cordio/pal_types.h" instead of "pal_types.h" if that's what components inside Mbed OS do?

Yes, but they'd have to actually reorganize their headers into such a directory in the first place - at the minute it's just a rather vague "stack/platform/include/pal_types.h".

Alternatively they could avoid any dependency on include path by doing explcitly #include "../../../include/pal_types.h" to find it from current directory before hitting the search path.

But don't forget the problem can happen in either direction - the #include "pal_types.h" in the client code could also mistakenly get Cordio's header. Which way the problem occurs is based on the alphabetical order. So both ends would want to have namespacing.

I quite like my idea of sorting include order by distance - I'm sure there's a viable metric that makes the correct header closer to the users.

Basically sort first by how many directories do you need to go up, then by how many you go down, from source folder to include folder?

Actually, in this case, they are doing #include "stack/platform/include/pal_types.h" consistently, so is it actually the client code's #include "pal_types.h" hitting theirs? Client could make the change here (to #include "PAL-Impl/Services-API/pal_types.h").

ciarmcom commented 4 years ago

Thank you for raising this detailed GitHub issue. I am now notifying our internal issue triagers. Internal Jira reference: https://jira.arm.com/browse/IOTOSM-2135

pan- commented 3 years ago

I'm closing this, the CMake build system resolves this issue where files external to the mbed-os tree with duplicate names are included in end user applications.