Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OpenThread apps want to download and build OpenThread every time! #12206

Closed
andyross opened this issue Dec 21, 2018 · 11 comments
Closed

OpenThread apps want to download and build OpenThread every time! #12206

andyross opened this issue Dec 21, 2018 · 11 comments
Assignees
Labels
area: OpenThread area: West West utility Enhancement Changes/Updates/Additions to existing features priority: low Low impact/importance bug

Comments

@andyross
Copy link
Contributor

A default build of apps that use openthread tries to clone and build the openthread project from github as part of the app directory! This gets sucked in by sanitycheck in a few places, for example samples/net/echo_server has two cases that overlay an openthread configuration into kconfig.

The immediate problem with this is that openthread dependencies like autotools have automatically become Zephyr dependencies. These aren't part of the Zephyr required tooling as we document it, so a pristine configured build host will fail on these tests (our CI infrastructure does seem to pull in autotools, unsurprisingly, so we don't see the problem there).

Obviously this means it's not possible to get a successful sanitycheck run to complete on a host without internet access.

It also breaks "git clean", which (even with the -f switch) will refuse to remove external git repositories it finds in the tree. My personal scripting tripped over this.

And obviously it's just a moderate to severe performance problem. By my count a default sanitycheck run right now appears to be downloading and building the same code FOUR TIMES every time we run it. Each time the process (downloading and configure scripts don't parallelize, needless to say) takes about 90 seconds for me, which is shockingly high for a Zephyr test case.

At the very least we need to update the documentation so that it correctly lists external dependencies and the CI infrastructure so that it includes a prebuilt copy of openthread (my personal CI docker image doesn't have it, so I assume the build hosts don't).

But personally I'd like to see this download step taken out of the Zephyr build entirely. That's just not the right way to do things.

@andyross andyross added the bug The issue is a bug, or the PR is fixing a bug label Dec 21, 2018
@aurel32
Copy link
Collaborator

aurel32 commented Dec 21, 2018

Instead of doing a git clone for each build, should we add a copy in ext/? Do we know if there is a reason for that not be done already? The original commit gives some explanations, but doesn't really cover that point:

    subsys: net: lib: Add OpenThread platform
    
    OpenThread requires platform definition with standarized API
    so we have to add wrappers to make it compatible with Zephyr.
    OpenThread is based on autoconf, this requires
    more specific CMakeLists.txt which allows to clone specific
    commit or point to local copy of openthread.
    
    Signed-off-by: Kamil Sroka <kamil.sroka@nordicsemi.no>

One possibility is that one has to run ./bootstrap before running configure to generate the automake/autoconf related files. Can we actually ship those in the ext/ directory with the openthread copy?

Probably a different issue, but right now the openthread code is built with -j99, which might render low end machine unresponsive for a few minutes.

@andyross
Copy link
Contributor Author

That seems reasonable. Or we could add it to the SDK. Or maybe come up with a framework for "pull and build these external source trees of this particular version" which could allow us to continue to reference the specific external stuff without deploying it ourselves, but in a way that could still be set up just once in CI.

@carlescufi
Copy link
Member

carlescufi commented Dec 23, 2018

@andyross @aurel32 please hold on for west. We are currently adapting it to new requirements but we plan to introduce it before LTS. It should solve this particular problem in a natural way, being part of the manifest.

@andyross
Copy link
Contributor Author

Would it be worth disabling the two OpenThread tests until that's ready? On a highly parallel build host those four download+configure cycles are adding like 30% to my build times. (I'm in a "sanitycheck is too slow" cycle at the moment)

@rlubos
Copy link
Contributor

rlubos commented Jan 3, 2019

I agree that the way how OpenThread is downloaded for each sample/build separately is not the most fortunate way to do this, waiting for west as suggested by @carlescufi sounds reasonable for now.

As for providing the pre-built OpenThread copies, note that the OpenThread library is configurable, and in Zephyr it's configuration can be altered by Kconfig. Hence I don't see how this could be done without giving up on the configurability (and it's not something that we want I believe).

Currently OT builds are triggerd for echo_client and echo_server samples. I think that we could safely limit the sanity tests for OpenThread to one of them as they do not differ in OpenThread configuration. I'd rather not disable sanity for OpenThread entirely though, as all in all it provides quick feedback when Zephyr - OT integration is broken.

@andyross
Copy link
Contributor Author

andyross commented Jan 3, 2019

Workaround submitted in #12280 as @rlubos suggested, which builds OpenThread just once for the nRF echo_client test. We can leave this open and re-enable the frdm and echo_server cases when west support lands.

@nashif nashif added the priority: low Low impact/importance bug label Jan 10, 2019
@ntavish
Copy link
Contributor

ntavish commented Feb 4, 2019

Just leaving a comment here for what needed to be done for building samples for OpenThread for me: automake and libtool need to be installed and I have to run bootstrap script from <build_dir>/zephyr/ext_proj/Source/ot, and then it would build successfully.

@nashif nashif added this to the future milestone Mar 3, 2019
@carlescufi carlescufi added Enhancement Changes/Updates/Additions to existing features and removed bug The issue is a bug, or the PR is fixing a bug labels Mar 6, 2019
@carlescufi
Copy link
Member

Marking as enhancement since this is actually an "optimization".

@nashif nashif removed this from the future milestone May 21, 2019
@mbolivar
Copy link
Contributor

mbolivar commented Oct 22, 2019

@carlescufi @rlubos can this be closed? Didn't the work done around the time of c6c20f0 fix the issue?

@aurel32
Copy link
Collaborator

aurel32 commented Oct 22, 2019

I confirm that the openthread module now contains the openthread code, so it is not downloaded anymore for each build.

@rlubos
Copy link
Contributor

rlubos commented Oct 23, 2019

Yeah, I guess we can close it.

@rlubos rlubos closed this as completed Oct 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: OpenThread area: West West utility Enhancement Changes/Updates/Additions to existing features priority: low Low impact/importance bug
Projects
None yet
Development

No branches or pull requests

8 participants