[PATCH wayland] wayland-egl: Ignore underscored symbols in ABI check

[Emil Velikov]

On 20 March 2018 at 15:09, Derek Foreman <derekf at osg.samsung.com> wrote:
> On 2018-03-20 10:02 AM, Emil Velikov wrote:
>> On 20 March 2018 at 14:50, Derek Foreman <derekf at osg.samsung.com> wrote:
>>> On 2018-03-20 07:11 AM, Daniel Stone wrote:
>>>> On 20 March 2018 at 11:55, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>>>> On 20 March 2018 at 11:46, Daniel Stone <daniel at fooishbar.org> wrote:
>>>>>> Sure. As on IRC though, we definitely need to add at least _ftext for
>>>>>> MIPS anyway:
>>>>>> https://gitlab.gnome.org/GNOME/glib/commit/ad12142943e0f20ed9583c9d6bf50f6262110c74
>>>>>>
>>>>>> And probably some more for ARM toolchains using other linkers:
>>>>>> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0474k/pge1362066045758.html
>>>>>>
>>>>>> Doing a quick check across all the architectures on Debian shows that
>>>>>> your updated list is also missing _fbss, _fdata, and _ftext. So we'd
>>>>>> need to respin for those as well, but I think at this point the
>>>>>> inconvenience of maintaining a list of every linker's implementation
>>>>>> detail on every platform, outweighs the risk of an exported
>>>>>> underscore-prefixed symbol slipping through review.
>>>>>
>>>>> I think you're preemptively worried about this. Plus there's no need
>>>>> to add every symbol a search could find.
>>>>>
>>>>> Even then, ask yourself the question:
>>>>> Which is better - updating an ugly looking list once in a blue moon or
>>>>> having apps use internal API?
>>>>>
>>>>> As said above: _if_ it turns out to be labour intensive - fine remove it.
>>>>> We've already spend more time researching than what the [expected]
>>>>> maintenance for 1 year would be ;-)
>>>>
>>>> It all depends on how you view scope and probability.
>>>>
>>>> I don't agree my concern is preemptive: we've just shipped a beta
>>>> where 'make check' fails on a few architectures, and the proposed fix
>>>> (adding several symbols to the platform list) will still fail on
>>>> architectures Debian ships on, until it also adds _fbss, _fdata and
>>>> _ftext. I think my suggestion is reasonable: it fixes the very real
>>>> problem, in a way which avoids expanding the scope of our ABI checker
>>>> to include the implementation details of every linker/architecture
>>>> pair that people use Wayland on. The cost of this is the probability
>>>> that someone manages to add a new underscored symbol marked with
>>>> WL_EXPORT into a library which is currently 105 LoC, and have no-one
>>>> notice that in review. I think that's a reasonable tradeoff.
>>>
>>> I'm inclined to agree with Daniel here.
>>>
>>> Someone managing to sneak a WL_EXPORT on a symbol starting with a _ past
>>> review seems reasonably unlikely.
>>>
>>> And after reading this thread I'm still not entirely sure we have a
>>> complete list of what should be on that whitelist, and there are only a
>>> limited number of systems I can test build on right now.
>>>
>> Sharing a few ideas that are less obvious. Having the partial list
>> allows you to:
>>  - see active developers/users - be that by merging their patches or
>> skimming through the report
>
> I don't imagine we'll be adding new API to that library frequently
> enough to mine that data productively.
>
I am talking about the C runtime symbols.

>>  - have contact point for people with unusual platforms/setups
>
> By defaulting to a broken build state for unusual platforms, do you
> really think we'll attract many contact points?
>
Quite a serious typo here - s/build/check/. From experience - yes, you will.

>>  - is fairly trivial newbie task ;-)
>
> I don't think we should be going out of our way to generate problems for
> newbies to solve.  There are plenty of worthwhile tasks for new
> participants already.
>
Might want to make the list more obvious. I've been [sort of] around
and haven't seen any.

> I've confirmed that we once again pass make check on arm (something I
> really should've done prior to the beta release.  sigh.) and have pushed
> this patch.
>
You can never have 'enough' tests before a release ;-)

-Emil