Larch platform #15
Larch platform #15
Conversation
.. Now it uses "origin" and "elem" fields, instead of a deprecated "element" field
.. (It builds successfully without them)
For some reason it was `ifname` in Broadcom implementation, although this field doesn't appear anywhere in SONIC YANG models
src/docker/manifest.json
Outdated
| }, | ||
| "service": { | ||
| "name": "ucentral_client", | ||
| "requires": [], | ||
| "after": [], | ||
| "after": ["gnmi", "mgmt-framework", "swss", "syncd"], |
There was a problem hiding this comment.
BRCM Sonic doesn't seem to be having explicit GNMI service;
Actually, i think we should try and separate manifests for each different target platform, and we should not try to fit a single one for all supported platforms;
| @@ -13,7 +13,7 @@ netlink/netlink.full.a: | |||
|
|
|||
| %.o: %.c | |||
| ifdef PLATFORM_REVISION | |||
| gcc -c -o $@ ${CFLAGS} -I ./ -I ../../include -D PLATFORM_REVISION='"$(PLATFORM_REVISION)"' $^ | |||
| gcc -c -g -o $@ ${CFLAGS} -I ./ -I ../../include -D PLATFORM_REVISION='"$(PLATFORM_REVISION)"' $^ | |||
There was a problem hiding this comment.
why do we need to explicitly generate debug info for each type of build? This flag has to be controlled from higher call tree perspective, i think;
Especially since these flags are unconditionally changed for existing platform
| status = (*gs->stub)->Get(&context, greq, &gres); | ||
|
|
||
| /* | ||
| * We don't have the implementation of JWT authentication, so disable it |
There was a problem hiding this comment.
You shouldn't be changing implementation of existing platform;
I'm still looking through code, but if you're using this implementation in Larch-plat code as well, we have to come up with some layering to maybe separate potential common code or something like that
src/ucentral-client/proto.c
Outdated
|
|
||
| cJSON *servers = cJSON_GetObjectItemCaseSensitive(s, "servers"); | ||
| if (!cJSON_IsArray(servers)) | ||
| { |
There was a problem hiding this comment.
Let's respect existing common code codestyle;
e.g. use
if (xx) {
P.S. same applies to all occurances;
src/ucentral-client/proto.c
Outdated
| goto err; | ||
| } | ||
|
|
||
| const cJSON *server_json = NULL; |
There was a problem hiding this comment.
Please move the declaration to top of the function;
P.S. same applies to all occurances;
src/docker/Dockerfile
Outdated
| @@ -9,6 +11,9 @@ RUN apt-get update && apt-get install --no-install-recommends -y \ | |||
| curl \ | |||
| libjsoncpp-dev \ | |||
| busybox \ | |||
| gdb \ | |||
There was a problem hiding this comment.
Let's keep debugging stuff out of release (default) builds
|
Hi @kburavchenko, Viacheslav, I was a bit carried away with jumping into reviewing the changes, and - where are my manners - I forgot to write a follow up comment on the changes; First of all, thank you for this PR and interest in upstreaming the changes, especially extending it beyond just your platforms implementation, but also expanding that onto introducing changes in the Proto part as well - any interested vendor could easily pull generic parser / internal structures population and filling code that would most certainly ease their implementation of their own platform. Secondly, the PR looks really good. I can see that the integration with base / common code and build instructions wasn't that complicated, and seems mostly clean and transparent. I've left few comments that are related only to the base code and common part that could be shared among different vendor's implementations, i didn't really peek into your platform code and how it works, I do belive you have a better vision on how it works, e.g. you know your best in that regard :) I hope the comments i've left make sense and you do agree on the changes required, if not we can proceed on and discuss the changes here, if you want. Thank you once again for these changes, and hopefully we will see Larch platform implementation available in this repo, main branch in the near time :) |
|
Hi @Cahb, thank you for a great review. All comments are useful and we will try to address them. |
…ainer" This reverts commit 0d7a532.
This reverts commit 58d3219.
This reverts commit c0f92db.
Parse switch.jumbo-frames instead of switch.properties.jumbo-frames (according to uCentral schema)
…s those .. .. requirements are not crossplatform across different platforms
Review fixes for uCentral upstream contribution See merge request openlan/ols-ucentral-client!39
kburavchenko
force-pushed
the
larch-platform
branch
from
1b5664a to
fa325f2
Compare
|
@Cahb please review again. |
| @@ -1,4 +1,4 @@ | |||
| FROM debian:buster | |||
| FROM arm64v8/debian:buster | |||
There was a problem hiding this comment.
Do we really need to specify platform-specific docker image in the common (builder) dockerfile?)
I've tried these changes, and for BRCM (x86 platform) i gave up compliing image after ~45 minutes
(Without this line that specifies platform in the docker image it took me ~15-20 minutes clean build till completion)
I think the platform specific img version in my case falls back to using qemu and hence slows down the compile process on the host OS;
Root dockerfile is the image... do you really need to use arm64 here? Is it expected that you'd be generating an image / deb package on the ARM system?
There was a problem hiding this comment.
<Discussed offline through slack, ignore this comment>
No description provided.