ot-br-posix icon indicating copy to clipboard operation
ot-br-posix copied to clipboard

Don't use cached version strings if re-installing OTBR

Open suveshpratapa opened this issue 3 years ago • 7 comments

Force re-evaluation of OTBR version strings (both OTBR version for otbr-agent daemon and OT version for the POSIX stack) if not supplied by the user. This prevents them from using previously cached values from CMakeCache in case the project is being re-built over a previously installed / cached version.

suveshpratapa avatar Aug 11 '22 18:08 suveshpratapa

Requesting review @jwhui @simonlingoogle ... we want someone that accidentally updates the OTBR install (without cleaning build folders) to have the accurate version string.

suveshpratapa avatar Aug 12 '22 13:08 suveshpratapa

Not being an expert on cmake, what happens when we build with -DOTBR_VERSION=1.0 and then without specifying -DOTBR_VERSION? @suveshpratapa

simonlingoogle avatar Aug 12 '22 13:08 simonlingoogle

Not being an expert on cmake, what happens when we build with -DOTBR_VERSION=1.0 and then without specifying -DOTBR_VERSION? @suveshpratapa

When a version is not specified, we have a calculation to insert the right version with git information included.

In your example, the user is specifying a "1.0" version that would then get cached in CMakeCache. When the user re-builds without specifying a version:

  • Prior to my change, it would use the cached version.
  • After my change, it re-evaluates the version the proper way that it would if one didn't provide a version (so includes git information etc.)

suveshpratapa avatar Aug 12 '22 13:08 suveshpratapa

  • After my change, it re-evaluates the version the proper way that it would if one didn't provide a version (so includes git information etc.)

IIUC, that's true only when building OTBR using script/setup. It does not apply for normal cmake builds (e.g. using ./script/cmake-build). Is that right?

simonlingoogle avatar Aug 12 '22 14:08 simonlingoogle

  • After my change, it re-evaluates the version the proper way that it would if one didn't provide a version (so includes git information etc.)

IIUC, that's true only when building OTBR using script/setup. It does not apply for normal cmake builds (e.g. using ./script/cmake-build). Is that right?

I've not run it with ./script/cmake-build directly but it is called as part of ./script/_otbr which gets run as part of ./script/setup. If someone is running ./script/cmake-build directly, it seems they would have to clean up the CMakeCache or specify the version again in the OTBR string arguments to cmake-build.

In this scenario, the ./script/setup process has a wrapper to take care of this (This is the documented way to install the OTBR, which is currently used by a lot of customers as well AFAIK).

suveshpratapa avatar Aug 12 '22 14:08 suveshpratapa

@simonlingoogle @jwhui Any other thoughts on this PR? We'd like to make sure to rely on accurate version on any OTBR installation. We have noticed that re-install or update of an existing setup can result in unreliable version strings.

suveshpratapa avatar Aug 15 '22 04:08 suveshpratapa

  • After my change, it re-evaluates the version the proper way that it would if one didn't provide a version (so includes git information etc.)

IIUC, that's true only when building OTBR using script/setup. It does not apply for normal cmake builds (e.g. using ./script/cmake-build). Is that right?

I've not run it with ./script/cmake-build directly but it is called as part of ./script/_otbr which gets run as part of ./script/setup. If someone is running ./script/cmake-build directly, it seems they would have to clean up the CMakeCache or specify the version again in the OTBR string arguments to cmake-build.

In this scenario, the ./script/setup process has a wrapper to take care of this (This is the documented way to install the OTBR, which is currently used by a lot of customers as well AFAIK).

Should we use Normal Variable if we don't like CACHE for this variable?

simonlingoogle avatar Aug 15 '22 05:08 simonlingoogle

Should we use Normal Variable if we don't like CACHE for this variable?

@simonlingoogle I tried this after your suggestion. For example, here's what I did with OTBR_VERSION (without any CACHE syntax):

set(OTBR_VERSION "")

However, after doing this, it still calculates the GitHub string even if the user provides their own version. For reference, I usually do so by:

sudo INFRA_IF_NAME=eth0 -DOTBR_VERSION=user_version_string ./script/setup

Any thoughts as to why this won't work with the Normal Variable change? I'm also a CMake novice. :)

suveshpratapa avatar Aug 16 '22 13:08 suveshpratapa