OpenXT / openxt

Base OpenXT build scripts
48 stars 39 forks source link

[build-scripts] Improve BUILD_ID and BUILD_DIR generation #199

Closed dozylynx closed 7 years ago

dozylynx commented 8 years ago

By default, build ID was not including the unique build directory suffix, so all builds on the same date would have the same build ID.

Take this opportunity to tidy up the Build ID and Build directory assignment logic.

Signed-off-by: Christopher Clark christopher.clark6@baesystems.com

jean-edouard commented 8 years ago

Currently building with this on my dev machine, no problem so far. Will merge if everything goes well.

jean-edouard commented 8 years ago

I could see a potential issue with the Windows build though, that uses the build ID as an integer... https://github.com/OpenXT/openxt/blob/master/windows/winbuild-prepare.ps1#L96

Could that be a problem?

jean-edouard commented 8 years ago

My build finished, but fails to boot because of something else I included...

To be honest though, I don't think this PR is a good idea... It doesn't really fix anything, and will break everything that uses the build ID as ... an ID (a number).

If we really have to address this, maybe we should rethink the ID generator to produce locally unique integers.

dozylynx commented 8 years ago

Hmm, that's pretty unfortunate on that cast to integer on the Windows build. I'd really like to be able to use the build ID as ... an ID (a unique-within-some-scope identifier that actually identifies a build).

I'll have another look at this and see if there's another simple approach that produces integers that are unique enough within a given build environment.

dozylynx commented 8 years ago

New revision pushed to the branch.

jean-edouard commented 8 years ago

Wow!! I just thought you'd remove the dash or something! This is very complicated, I can't figure it out on first read... Could you please explain what the code does?

dozylynx commented 8 years ago

This logic ensures that two variables have been populated:

BUILD_ID can be supplied by the user as argument to the build script (-i parameter) or left unspecified for the build script to generate.

Similarly BUILD_DIR can also be supplied by the user as argument to the build script (-n parameter) or left unspecified for the build script to generate.

This gives four cases for the logic to handle:

  1. Build ID specified, Build Dir specified
  2. Build ID specified, Build Dir not specified
  3. Build ID not specified, Build Dir specified
  4. Build ID not specified, Build Dir not specified

and you can see the structure that follows from that in the patch:

if [ -z "${BUILD_ID}" ] ; then
    if [ -z "${BUILD_DIR}" ] ; then
...
    else
...
    fi
else
    if [ -z "${BUILD_DIR}" ] ; then
...
    fi
fi

The technique used for generating unique-enough build identifiers that will work for multiple local build environments is to store a counter in the local git mirror's openxt repository. There are benefits to this:

The choice to use openxt.git was because this is the repository containing the build scripts being used.

Care is taken to ensure that an existing build directory is not reused unless explicitly directed by the user.

In a similar fashion to the prior version, in the absence of any previous build, an ID is generated from the current date.

So, the four cases:

1) Build ID specified, Build Dir specified

Easy: nothing to do, values are already assigned.

2) Build ID specified, Build Dir not specified

Script needs to assign the BUILD_DIR.

If there is no existing directory matching the build ID specified, use the build ID as the new build directory name; otherwise generate a unique suffix to the specified build ID and use that.

3) Build ID not specified, Build Dir specified

Script needs to assign the BUILD_ID.

If the specified build directory name is a six digit number suitable to use as a BUILD_ID, then set BUILD_ID to that;

if not, generate a new ID using the stored counter.

4) Build ID not specified, Build Dir not specified

Generate a new ID using the stored counter, avoiding any existing build directories, and use it for both BUILD_ID and BUILD_DIR.

jean-edouard commented 8 years ago

That's not how it was designed... -i is supposed to be incompatible with -n (which is stated in the help).

build.sh creates a new build by default, -i gives it a custom name. -n overrides the default behavior by continuing a build instead of creating a new one. So case "1." should be an error.

The problem you're trying to solve with many lines of very complicated code could also be fixed by just removing a dash. I think we should go with that instead...

jean-edouard commented 8 years ago

Let me give it a shot. I think this is a chance to simplify the scripts, not complicate them.

jean-edouard commented 8 years ago

Something like: https://github.com/jean-edouard/openxt/commit/b93bd466195fbc95ee66260ebee40f25cbbf608d (untested)