Thursday, 13 December 2018

Nightly build steps with Jenkins declarative pipeline

Jenkins declarative pipeline is a new(ish) pipeline syntax for Jenkins that allows you to write pipeline-as-code in a Jeninsfile. Unlike a traditional pipeline, instead of writing Groovy code, a declarative pipeline uses a custom DSL. A simple example that builds a C++ application is below:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
pipeline {
    agent any
    stages {
        stage('Clean Old Builds') {
            steps {
                sh 'rm -fr _builds'
            }
        }
 
        stage('Build') {
            sh 'cmake -H. -B_builds/default'
            sh 'cmake --build _builds/default'
        }
 
        stage('Test')
            sh 'cmake --build _builds/default --target test'
        }
    }
}

The above example, will have 3 stages that can run on any build machine.

  • Stage 1, Clean Old Builds, will remove any old builds.
  • Stage 2, Build, will run cmake and make.
  • Stage 3, Test, will run any unit tests.

This can be cleaner and easier to read than the older Groovy based pipeline syntax.

Nightly Builds

In a previous post, I discussed how to run nightly builds using Jenkins Groovy pipelines. I tried to use the same isJobStartedByTimer function with declarative pipeline, as follows,

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
def isTimerBuild = isJobStartedByTimer()
pipeline {
...
    triggers {
        cron(env.BRANCH_NAME == 'master' ? 'H H(0-2) * * *' : '')
    }
    stages {
        ...
        stage('Nightly') {
            when {
                expression {
                    isTimerBuild == true
                }
            }
            steps {
                sh 'cmake --build _builds/default --target cppcheck
            }
        }
    }
}
// define isJobStartedByTimer function as before
@NonCPS
def isStartedByTimer() {
  ....
}

Unfortunately this failed because of an unauthorized error. I was unable to add the required values to the In Process Script Approval as they never appeared in the approval window.

Improved Method

While looking for a solution to the above problem I came across the following feature request in Jenkins. This was added in the pipeline workflow api plugin in v2.22. The core of this change is that it allows access to the build causes via the currentBuild class, instead of the sandboxed currentBuild.rawBuild class.

1
2
currentBuild.getBuildCauses()
currentBuild.getBuildCauses(String superClassName)

When using these functions you do not have to enable any In Script approvals. To get this to work I updated my isJobStartedByTimer as follows:

1
2
3
4
5
6
7
8
9
10
11
12
@NonCPS
def isJobStartedByTimer() {
    try {
        if(currentBuild.getBuildCauses('hudson.triggers.TimerTrigger$TimerTriggerCause').size() > 0) {
            echo "build started by timer"
            return true
        }
    } catch(theError) {
        echo "Error getting build cause: ${theError}"
    }
    return false
}

Using this updated function, and the example pipeline from earlier, allows me to have build steps that only run at night.

Alternative Improved Method

In some cases I found that the above improved method caused a ClassNotFoundException saying that the TimerTrigger class did not exist. An alternative example, that could be used if you have receive this error, is to use the currentBuild.getBuildCauses() version of the function. This works as follows:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
@NonCPS
def isJobStartedByTimer() {
    try {
        for ( buildCause in currentBuild.getBuildCauses() ) {
            if (buildCause != null) {
                if (buildCause.shortDescription.contains("Started by timer")) {
                    echo "build started by timer"
                    return true
                }
            }
        }
    } catch(theError) {
        echo "Error getting build cause: ${theError}"
    }
    return false
}

Thursday, 6 December 2018

Cppcheck: More Checks

In my previous posts, I discussed some errors that Cppcheck can help you find. In this post I'm going to look at some more errors that Cppcheck can prevent.

Memory Leak from realloc

When using realloc, a failure to allocate data will return a null pointer. This can lead to a common error where the original data that you tried to reallocate is leaked. For example:

1
2
3
4
5
6
7
void realloc_issue() {
    int* data = (int*)malloc(4 * sizeof(int));
    // do something
    data = (int*)realloc(data, 8*sizeof(int));
    // do something
    free(data);
}

In the above code, the original data variable is leaked if the call to realloc fails. Using Cppcheck you will get the following warning about the code:

1
[realloc.cpp:4]: (error) Common realloc mistake: 'data' nulled but not freed upon failure

To fix this error you should allow a temporary to take the returned result from realloc and only assign it to data if it is valid memory:

1
2
3
int* new_data = (int*)realloc(data, 8*sizeof(int));
if(new_data)
    data = new_data;

Invalidated Iterator

When using stl containers such as std::vector, using methods such as push_back and resize can invalidate iterators. Cppcheck has a check which can help to catch this mistake. For example:

1
2
3
4
5
6
7
8
9
10
11
void pb_it() {
    std::vector<std::string> v;
    v.push_back("first");
 
    std::vector<std::string>::iterator first = v.begin();
    for(int i = 0; i < 100; ++i) {
        v.push_back("loop");
    }
 
    std::cout << *first << "\n";
}

This will result in the following warning:

1
[push_back.cpp:10]: (error) After push_back(), the iterator 'first' may be invalid.

Note: If you use auto for the iterator or a standard algorithm to fill your vector, then Cppcheck will fail to warn about the error. For example, neither of the following will produce a warning:

1
2
3
std::vector<std::string>::iterator first = v.begin();
std::fill_n(std::back_insertor(v), 100, "loop");
std::cout << *first << "\n";

or

auto first = v.begin(); for ... std::cout << *first << "\n";

Invalid Operation on File

Using the C library functions for working on a FILE*, there is no type safe way at compile time to catch code that reads from a write only file, or writes to a read only file. However, Cppcheck has warnings which can catch this error.

1
2
3
4
5
6
7
8
9
10
11
12
13
void read_on_write_only() {
    FILE* f = fopen("xxxw", "w");
    std::array<char, 100> buf;
    fread(buf.data(), 1, 100, f);
    fclose(f);
}  
 
void write_on_read_only() {
    FILE* f = fopen("xxxf", "r");
    int x = 0;
    fwrite(&x, sizeof(int), 1, f);
    fclose(f);
}  

In the above code we use fread on a file that was opened as write only, and fwrite on a file that was opened read only. Cppcheck will output the following warnings:

1
2
[file_ops.cpp:4]: (error) Read operation on a file that was opened only for writing.
[file_ops.cpp:11]: (error) Write operation on a file that was opened only for reading.

Note: This doesn't warn with std::fstream. For example, If you open a file with ios_base::in, and try to write to it then Cppcheck will fail to warn.

Class Checks

Cppcheck has a number of both style and warning checks that can hint at potential problems with your class design. For example, take this poorly designed class:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
#include <string>
 
class Multipler
{
public:
    Multipler(int x) : x_(new int(x))
    {
    }
    ~Multipler() = default;
    Multipler& operator=(const Multipler& rhs) {
        x_ = new int(*rhs.x_);
        return *this;
    }
    int multiply(int y) const { return y * (*x_); }
 
    void name(const std::string& name) { name_ = name; }
private:
    int* x_;
    std::string name_;
    int vx_;
};

On most compilers this will compile with no warnings. However, Cppcheck will output the following warnings:

1
2
3
4
5
6
7
[class_design.cpp:6]: (warning) Member variable 'Multipler::vx_' is not initialized in the constructor.
[class_design.cpp:10]: (warning) Member variable 'Multipler::vx_' is not assigned a value in 'Multipler::operator='.
[class_design.cpp:10]: (warning) 'operator=' should check for assignment to self to avoid problems with dynamic memory.
[class_design.cpp:6]: (style) Class 'Multipler' does not have a copy constructor which is recommended since it has dynamic memory/resource allocation(s).
[class_design.cpp:6]: (style) Class 'Multipler' has dynamic memory/resource allocation(s). The destructor is explicitly defaulted but the default destructor does not work well. It is recommended to define the destructor.
[class_design.cpp:6]: (style) Class 'Multipler' has a constructor with 1 argument that is not explicit.
[class_design.cpp:11]: (warning) Possible leak in public function. The pointer 'x_' is not deallocated before it is allocated.

These are good checks, however, you need to have style warnings enabled for some of them. If you only use enable=warning, Cppcheck will not warn about the fact that you fail to delete x_ in the destructor.

Conclusions about Cppcheck

Cppcheck is a good tool to help you write more robust and safe code. However, it is not a silver bullet to prevent errors. As noted in some of my examples, a small change to your code can mean that Cppcheck will fail to catch an error.

I have found that Cppcheck can be very helpful when taking over an old codebase that uses an older style of C++. After enabling Cppcheck, you can often catch errors that have existed in the code for years and the developers have been lucky not to hit (e.g. mistakes in error handlers). If you are using more modern C++ style, Cppcheck might not be up to date with all changes and may miss some warnings. However, I would still recommend to have it running in your CI system to prevent errors from reaching your production code.