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

Remove Travis CI #103

Merged
merged 7 commits into from
Apr 30, 2021
Merged

Remove Travis CI #103

merged 7 commits into from
Apr 30, 2021

Conversation

IshaanDesai
Copy link
Member

This PR removes Travis CI as the mock tests run in the CI are now run as a GitHub Actions job

Copy link
Member

@BenjaminRodenberg BenjaminRodenberg left a comment

Choose a reason for hiding this comment

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

Removing this test is good. As far as I undestand the corresponding github actions test is this one:

setup_test:
name: Run setup test
runs-on: ubuntu-latest
container: precice/ci-ubuntu-1804
steps:
- name: Checkout Repository
uses: actions/checkout@v2
- name: Install & upgrade pip3
run: |
apt-get -yy update
apt-get install -y python3-pip
rm -rf /var/lib/apt/lists/*
pip3 install --upgrade --user pip
- name: Checkout precice and make required files discoverable
run: |
git clone https://github.com/precice/precice.git precice-core
mkdir -p precice
cp precice-core/src/precice/SolverInterface.hpp precice/SolverInterface.hpp
- name: Install dependencies
run: |
pip3 install --user toml
python3 -c 'import toml; c = toml.load("pyproject.toml"); print("\n".join(c["build-system"]["requires"]))' | pip3 install -r /dev/stdin
- name: Run setup test
run: |
export CFLAGS=-I$GITHUB_WORKSPACE
python3 setup.py test

I still see a minor problem here: The old test on travis uses a generic docker image as basis (probably some ubuntu?). The github-actions-based test uses precice/ci-ubuntu-1804. If possible, please use ubuntu:20.04 or a similarly simple image as basis, since otherwise this contradicts the idea of mocking a bit. You can use the fenics-adapter mock test as a reference. Here, container: quay.io/fenicsproject/stable:current provides fenics, but all the other dependencies of preCICE and preCICE itself can be ignored and are therefore not present.

@IshaanDesai
Copy link
Member Author

I see the problem with using the container precice/ci-ubuntu-1804, it does indeed violate the broad mock testing principle. But in the test/SolverInterface.cpp we do import the original preCICE header file here:

#include "precice/SolverInterface.hpp"

In my understanding this requires the container to have the preCICE compiled or at least the code be present there. Is this correct? If the GitHub action is run on just a ubuntu container then the following error is seen:

| /home/desaiin/python-bindings/cyprecice/cyprecice.cpp:671:10: fatal error: precice/SolverInterface.hpp: No such file or directory
|   671 | #include "precice/SolverInterface.hpp"
|       |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| compilation terminated.
| error: command 'x86_64-linux-gnu-gcc' failed with exit status 1
[Build and Test/Run setup test]   ❌  Failure - Run setup test

@BenjaminRodenberg
Copy link
Member

I see the problem with using the container precice/ci-ubuntu-1804, it does indeed violate the broad mock testing principle. But in the test/SolverInterface.cpp we do import the original preCICE header file here:

#include "precice/SolverInterface.hpp"

In my understanding this requires the container to have the preCICE compiled or at least the code be present there. Is this correct? If the GitHub action is run on just a ubuntu container then the following error is seen:

| /home/desaiin/python-bindings/cyprecice/cyprecice.cpp:671:10: fatal error: precice/SolverInterface.hpp: No such file or directory
|   671 | #include "precice/SolverInterface.hpp"
|       |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| compilation terminated.
| error: command 'x86_64-linux-gnu-gcc' failed with exit status 1
[Build and Test/Run setup test]   ❌  Failure - Run setup test

precice/SolverInterface.hpp is needed. Yes. But the rest of preCICE is not needed. The idea here is the following:

  • Use the real precice/SolverInterface.hpp to ensure that the interface that we are testing is the real preCICE interface
  • Use a fake SolverInterface.cpp as implementation.

The important part is here:

- name: Checkout precice and make required files discoverable
run: |
git clone https://github.com/precice/precice.git precice-core
mkdir -p precice
cp precice-core/src/precice/SolverInterface.hpp precice/SolverInterface.hpp

We clone precice, since we need precice/SolverInterface.hpp and then move it to a location, where the compiler can discover it (precice/SolverInterface.hpp). preCICE itself is never installed, all other header files are missing.

If we then want to build test, we do not link against the real preCICE (it does not exist on the system), but just provide the fake SolverInterface.cpp as sources:

python-bindings/setup.py

Lines 72 to 76 in 6e54396

if not is_test:
link_args.append("-lprecice")
if is_test:
bindings_sources.append(os.path.join(PYTHON_BINDINGS_PATH, "test",
"SolverInterface.cpp"))

I hope this explanation helps. It's not documented well and I assume must be very confusing, if you don't know where you have to look.

Copy link
Member

@BenjaminRodenberg BenjaminRodenberg left a comment

Choose a reason for hiding this comment

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

Looks good. I will quickly check and apply the minor things I've spotted and then directly merge this PR.

Comment on lines +77 to +81
- name: Install & upgrade pip3 and install OpenMPI
run: |
apt-get -yy update
apt-get install -y python3-pip
rm -rf /var/lib/apt/lists/*
sudo apt-get -yy update
sudo apt-get install -y python3-pip libopenmpi-dev
sudo rm -rf /var/lib/apt/lists/*
Copy link
Member

Choose a reason for hiding this comment

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

Related to #101: We should be able to remove the mpi part here.

But for the current state everything is fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, will keep this dependency problem in mind.

.github/workflows/build-and-test.yml Outdated Show resolved Hide resolved
@BenjaminRodenberg BenjaminRodenberg merged commit 2d40afa into develop Apr 30, 2021
@BenjaminRodenberg BenjaminRodenberg deleted the remove-travis branch April 30, 2021 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants