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

Feature/devcontainer #731

Open
wants to merge 37 commits into
base: master
Choose a base branch
from
Open

Feature/devcontainer #731

wants to merge 37 commits into from

Conversation

pnoltes
Copy link
Contributor

@pnoltes pnoltes commented Feb 19, 2024

This PR introduces a Development Container setup.

Development Container:

A development container (or dev container for short) allows you to use a container as a full-featured development environment. It can be used to run an application, to separate tools, libraries, or runtimes needed for working with a codebase, and to aid in continuous integration and testing. Dev containers can be run locally or remotely, in a private or public cloud, in a variety of supporting tools and editors.

Source https://containers.dev

I think the development container setup is nicer that our current containers dir, because the development container is based on a specification and is currently already supported in Clion and VSCode.

I tried building and debugging individual tests using a dev container in CLion and VSCode and both worked for me.
IMO the support for dev containers work better in VSCode. Also - if I am correct - VSCode has now a better CMake support and combined with the C++ TestMate plugin I think I will starting using VSCode more.

@pnoltes
Copy link
Contributor Author

pnoltes commented Feb 19, 2024

@rlenferink Could you have a look and see if the devcontainer setup works for you?

If you agree I would like to remove the container dir and gitpod badge in favor for the devcontainer setup.

@codecov-commenter
Copy link

codecov-commenter commented Jun 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.26%. Comparing base (758aa79) to head (f9c22aa).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #731      +/-   ##
==========================================
+ Coverage   90.25%   90.26%   +0.01%     
==========================================
  Files         226      226              
  Lines       26320    26320              
==========================================
+ Hits        23755    23759       +4     
+ Misses       2565     2561       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pnoltes
Copy link
Contributor Author

pnoltes commented Jun 23, 2024

I updated the devcontainer setup to use Ubuntu 24.04. For now, this is different from the Ubuntu CI. When the GitHub Ubuntu runners for 24.04 are no longer in beta, I would like to update our Ubuntu CI to 24.04 as well.

I also moved all the container setup to the .devcontainer directory.

conanfile.py Outdated Show resolved Hide resolved
@PengZheng
Copy link
Contributor

PengZheng commented Jun 24, 2024

There is one remaining test failure:

26/43 Test #26: run_unit_test_discovery_zeroconf .................***Failed    0.01 sec
/workspace/build/bundles/remote_services/discovery_zeroconf/gtest/unit_test_discovery_zeroconfd: /lib/x86_64-linux-gnu/libssl.so.3: version `OPENSSL_3.2.0' not found (required by /root/.conan2/p/b/libcu5cd8ac887cea2/p/lib/libcurl.so.4)

When the tests are run under sudo, the system openssl rather than the one in Conan cache will be used.
We don't need sudo any more in container, so I drop it.

After dropping sudo, run_unit_test_discovery_zeroconf will hang. Please have a look, @xuzhenbao

# Conflicts:
#	libs/framework/gtest/src/CelixLauncherTestSuite.cc
#	libs/framework/src/celix_launcher.c
@PengZheng
Copy link
Contributor

PengZheng commented Jun 24, 2024

Besides the sudo issue, there seems to be shaky tests:
https://github.com/apache/celix/actions/runs/9644957822/job/26598173196?pr=731

[ RUN      ] DiscoveryZeroconfWatcherTestSuite.BrowseServicesFailed1
[2024-06-24T12:13:10] [   info] [celix_framework] [framework_start:483] Celix framework started
[2024-06-24T12:13:10] [  trace] [celix_framework] [framework_start:484] Celix framework started with uuid f82db217-e714-4955-8bf9-94a775f5fa59
[2024-06-24T12:13:10] [  debug] [celix_framework] Framework started event received -> registering framework.ready condition service
[2024-06-24T12:13:10] [  error] [DiscoveryZeroconf] Watcher: Failed to create watched services map.
/workspace/bundles/remote_services/discovery_zeroconf/gtest/src/DiscoveryZeroconfWatcherTestSuite.cc:706: Failure
Value of: timeOut
  Actual: true
Expected: false
[2024-06-24T12:13:40] [  trace] [celix_framework] [celix_framework_shutdownAsync:1157] Start shutdown thread for framework f82db217-e714-4955-8bf9-94a775f5fa59
[2024-06-24T12:13:40] [  trace] [celix_framework] [celix_bundleContext_cleanup:116] Cleaning up bundle context `celix_framework` (id=0)
[2024-06-24T12:13:40] [  trace] [celix_framework] [framework_shutdown:1110] Celix framework shutting down
[2024-06-24T12:13:40] [  trace] [celix_framework] [celix_framework_stopAndJoinEventQueue:1095] Stop and joining event loop thread for framework f82db217-e714-4955-8bf9-94a775f5fa59
[2024-06-24T12:13:40] [  debug] [celix_framework] [celix_framework_stopAndJoinEventQueue:1104] Joined event loop thread for framework f82db217-e714-4955-8bf9-94a775f5fa59
[  FAILED  ] DiscoveryZeroconfWatcherTestSuite.BrowseServicesFailed1 (30004 ms)

@@ -53,11 +53,11 @@ static int GetLoopBackIfIndex(void);
class DiscoveryZeroconfAnnouncerTestSuite : public ::testing::Test {
public:
static void SetUpTestCase() {
(void)system(MDNSD);
(void)system("sudo " MDNSD);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

If we need to start processes from C more often (e.g. etcd, msnd, etc) maybe we should also think about running integration test with help of something like behave or avocado. I think it is even possible to bootstrap behave/avocado from cmake/ctest using a add_test

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll find some time to optimize the tests in another new PR.
I have tried to add FIXTURES in cmake yesterday, but due to some synchronization issues in the current unit tests, and I can't resolve them in a short time. so I'm going to start subprocess from C in this PR.

@@ -340,6 +339,7 @@ def requirements(self):
# https://github.com/conan-io/conan-center-index/pull/16254
self.requires("mdnsresponder/1310.140.1")
# 'libzip/1.10.1' requires 'zlib/1.2.13' while 'libcurl/7.64.1' requires 'zlib/1.2.12'
self.requires("openssl/[>=3.2.0]", override=True)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@pnoltes
Copy link
Contributor Author

pnoltes commented Jun 27, 2024

I think this PR is ready for review again. @rlenferink and/or @Deedss If able could you have a look at this PR?

Copy link

@Deedss Deedss left a comment

Choose a reason for hiding this comment

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

Overall LGTM, builts on Fedora 40 with SELinux enabled.

@pnoltes Please note that SELinux requires the presence of "securityOpt":["label=disable"] in the devcontainer.json to build. Otherwise SELinux blocks the execution of the conan scripts.

@rlenferink
Copy link
Member

@EyeDevelop do you feel interested (with your VScode knowsledge) to review this PR?

@pnoltes I’ll review it somewhere this week.

@rlenferink
Copy link
Member

@pnoltes I just tested this with Fedora + podman + CLion and at least for me this doesn't seem to work.

The steps I executed:

[rlenferink@fedora .devcontainer]$ git checkout feature/devcontainer
[rlenferink@fedora .devcontainer]$ cd .devcontainer
[rlenferink@fedora .devcontainer]$ ./build-devcontainer-image.sh
<snip>
[rlenferink@fedora .devcontainer]$ ./run-devcontainer.sh 
Do you want to mount the .gnupg directory to the container (as an overlayfs)? (yes/no): n
Do you want to mount the .gitconfig file to the container (as read-only)? (yes/no): no
Do you want to forward the SSH agent to the container? (yes/no): no
Starting container 'celixdev' with command: sudo /usr/sbin/sshd -D -e -p 2233
5a9d8ba9ffb813252bd6e77e67e97ad9da07ce364687e1e9a0e09ffbf8cc49dc

Do you want to build Celix dependencies with Conan? (yes/no): no
Done. You can connect with ssh using 'ssh -p 2233 celixdev@localhost' and password 'celixdev'

Then, using CLion to connect via SSH to the container succeeds, but editing files is impossible, since the files are marked as read-only.

Entering the container the error is indeed correct, since the rootless celixdev user is not allowed to write to the mounted volume.

[rlenferink@fedora .devcontainer]$ podman exec -it celixdev bash
celixdev@fedora:~/workspace$ touch test
touch: cannot touch 'test': Permission denied
celixdev@fedora:~/workspace$ ls -al
total 240
drwxr-xr-x. 1 rlenferink users       532 Jul 11 18:33 .
drwxr-x---. 1 celixdev   celixdev     66 Jul 11 18:43 ..
-rw-r--r--. 1 rlenferink users      2182 Jan  7  2024 .asf.yaml
-rw-r--r--. 1 rlenferink users      1616 Jan  7  2024 .clang-format
drwxr-xr-x. 1 rlenferink users       448 Jul 11 18:33 .devcontainer
drwxr-xr-x. 1 rlenferink users       226 Jul 11 18:33 .git
drwxr-xr-x. 1 rlenferink users        18 Jul 17  2022 .github
-rw-r--r--. 1 rlenferink users       949 Jan  7  2024 .gitignore
-rw-r--r--. 1 rlenferink users       988 Jan  7  2024 .gitpod.yml
drwxr-xr-x. 1 rlenferink users       146 Jan  7  2024 .idea
-rw-r--r--. 1 rlenferink users       534 Mar  7  2021 BUILDING
-rw-r--r--. 1 rlenferink users     22338 Jul 11 18:33 CHANGES.md
-rw-r--r--. 1 rlenferink users      9720 Jul 11 18:33 CMakeLists.txt
-rw-r--r--. 1 rlenferink users    107822 Apr 27  2023 Doxyfile
-rw-r--r--. 1 rlenferink users     11273 Mar  7  2021 KEYS
-rw-r--r--. 1 rlenferink users     18766 Jan  7  2024 LICENSE
-rw-r--r--. 1 rlenferink users       169 Jul 11 18:33 NOTICE
-rw-r--r--. 1 rlenferink users     10241 Jul 11 18:33 README.md
drwxr-xr-x. 1 rlenferink users       278 Jan  7  2024 build
drwxr-xr-x. 1 rlenferink users       206 Jul 11 18:33 bundles
drwxr-xr-x. 1 rlenferink users       250 Jul 11 18:33 cmake
drwxr-xr-x. 1 rlenferink users       196 Jan  7  2024 cmake-build-debug
drwxr-xr-x. 1 rlenferink users       196 Jan  7  2024 cmake-build-relwithdebinfo
-rw-r--r--. 1 rlenferink users       845 Jul 11 18:33 codecov.yml
-rw-r--r--. 1 rlenferink users     17174 Jul 11 18:33 conanfile.py
drwxr-xr-x. 1 rlenferink users        28 Apr 27  2023 doap
drwxr-xr-x. 1 rlenferink users       424 Jul 11 18:33 documents
drwxr-xr-x. 1 rlenferink users       134 Sep 23  2023 examples
drwxr-xr-x. 1 rlenferink users       164 Jan  7  2024 libs
drwxr-xr-x. 1 rlenferink users        60 Jul 17  2022 misc
-rw-r--r--. 1 rlenferink users      1665 Jan  7  2024 rat-excludes.txt

Am I doing something wrong here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants