[LEDE-DEV] [PATCH v2 2/3] base-files: put board_name into separate file

[Roman Yeryomin]

On 18 May 2017 at 01:05, Mathias Kresin <dev at kresin.me> wrote:
> 17.05.2017 21:08, Roman Yeryomin:
>>
>> On 17 May 2017 at 21:39, Roman Yeryomin <leroi.lists at gmail.com> wrote:
>>>
>>> On 17 May 2017 at 16:19, Mathias Kresin <dev at kresin.me> wrote:
>>>>
>>>> Hey Roman.,
>>>>
>>>> independent of the work done by you, I worked on exactly the same
>>>> topic. But I've pushed it a bit further by switching all targets to
>>>> the generic boardname function and a few targets to the generic board
>>>> detection in basefiles/preinit.
>>>>
>>>> Furthermore I've converted some targets to get the boardname from the
>>>> device tree compatible string as suggested by John. Till now I wasn't
>>>> comfortable to push it somewhere, but with my latest changes it should
>>>> work in theory. You can find the changes at
>>>>
>>>> https://git.lede-project.org/?p=lede/mkresin/staging.git;a=shortlog;h=refs/heads/boarddetection.
>>>> What is missing: testing!
>>
>>
>> Some of your changes would be ok to me if:
>> - board.sh would be used instead of functions.sh (which I would like
>> to split into several files anyway) for $(board_name)
>
>
> I guess we all got your point. But I'm sorry to say, that isn't subject of
> my changes.

I just wanted to say that the way you did it doesn't make sense to me
and IMO does more harm than good.

>> - related changes would be squashed into one commit, otherwise it's
>> hard to perceive/track the changes, IMO more commits is not always
>> better
>
>
> The commits will be most likely squashed before I commit them. But at least
> during review I prefer smaller changes instead of a single patch with >100
> lines.
>
>>
>> also this one is a fail:
>>
>> https://git.lede-project.org/?p=lede/mkresin/staging.git;a=commitdiff;h=af3ef20591e6e9e4ee2cf1b1fc58012df68445ba
>
>
> Hopefully someone beats me with a stick if I every reply during a review
> with "fail" and without any explanation what is wrong. This isn't helpful in
> any kind and just a waste of your and my time.

Sorry, didn't mean to be rude.
But, again, the way you did it doesn't make sense to me. You replaced
one platform specific code with other, it's not unifying anything,
like you wrote in commit message.
Instead here is what I propose: https://patchwork.ozlabs.org/patch/759976/

> But I can give you a quick summary why the commit is required:
>
> No target_board_name() => target_board_detect() isn't called =>
> /tmp/sysinfo/board_name isn't populated => board_name() has nothing to
> return.
>
> And as the commit message states, it is also meant to unify the way the
> board detection is done between targets (to not fool more people like I was
> fooled because of these target specifics).
>
> Mathias