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

Clean up Conan CI build environment using Docker and run tests in Conan CI builds #644

Merged
merged 3 commits into from
Aug 18, 2021

Conversation

kyllingstad
Copy link
Member

This change was motivated by some test failures in PR #633 which are almost surely unrelated to the PR itself. The probable diagnosis is simply that the build environment provided by GitHub's workflow runners is so messy that some unwanted library inadvertently gets picked up from outside the Conan package ecosystem. (We add to the mess by installing an additional GCC version, which happens to be the version for which the tests fail...)

Long story short, here I've set up a cleaner build environment using Docker. The building still takes place on GitHub's runners, but is now isolated from the messy host environment inside a Docker container. The base environments for the different compilers are the ones used for Conan Center.

The process, described in the Linux part of .github/workflows/ci-conan.yml, now goes like this:

  1. Generate /tmp/osp-builder-docker/Dockerfile , which defines the build environment as a Docker image.
  2. Generate /tmp/osp-builder-docker/entrypoint.sh, which gets copied into the Docker image as /entrypoint.sh and contains the actual libcosim build steps.
  3. Build a Docker image named osp-builder based on the above files.
  4. Run a Docker container based off osp-builder, i.e., run /entrypoint.sh inside the build environment. The source folder which is checked out on the host runner gets mounted read-only at /mnt/source inside the container.

I've included a change which was originally in #633, namely the addition of the environment variable LIBCOSIM_RUN_TESTS_ON_CONAN_BUILD, added so we can run tests in Conan-based CI builds too.

@kyllingstad kyllingstad added the enhancement New feature or request label Aug 17, 2021
@kyllingstad kyllingstad requested review from ljamt and markaren August 17, 2021 12:20
@kyllingstad kyllingstad self-assigned this Aug 17, 2021
@kyllingstad
Copy link
Member Author

@markaren, two notes for PR #633:

  • You probably want to re-enable the proxy tests in your PR once it's merged with this one, i.e. remove my line 62 in conanfile.py.
  • I changed the prefix of the new environment variable from COSIM_ to LIBCOSIM_ since that's the convention we've used elsewhere.

Copy link
Member

@ljamt ljamt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good :)

@markaren
Copy link
Contributor

Wupsie, seems tests are not enabled for windows (ENV not set)

@kyllingstad
Copy link
Member Author

Good catch! And enabling it revealed another problem: When environment variables are set to True in a workflow YAML file, it gets interpreted as a boolean rather than a string and converted to lowercase true during the parsing process. Conan only understands the literal values True and 1 for its environment variables, and true gets interpreted as false. :-|

This is probably an issue with the CMake CI setup too, so I'll investigate and open a separate PR for that if necessary.

@kyllingstad kyllingstad requested a review from markaren August 18, 2021 06:52
@kyllingstad kyllingstad merged commit 517dac9 into master Aug 18, 2021
@kyllingstad kyllingstad deleted the feature/dockerised-ci branch August 18, 2021 06:54
kyllingstad added a commit that referenced this pull request Aug 18, 2021
Conan only understands the values "True" or "1" for its configuration
variables.  However, in a YAML file, the value "True" gets intepreted as
a boolean rather than a string and converted to lowercase "true" at some
point by GitHub Actions. Conan does *not* understand that and interprets
it as "False" instead.

I don't think it's caused any actual problems here (unlike for the Conan
build which was fixed in PR #644), but it should still be fixed so the
code does what we think it does.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants