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

148 add devcontainer #163

Merged
merged 14 commits into from
Nov 21, 2024
Merged

148 add devcontainer #163

merged 14 commits into from
Nov 21, 2024

Conversation

alexallmont
Copy link
Contributor

I've tested this on Linux, Macos and Windows and also in a codespace in GitHub. It worked initially but when I changed some of the container parameters I hit an issue with the tests being stuck with a _NOT_BUILT prefix. I think this is due to an open vscode-cmake-tools issues 3358. For the time being I am using Ubuntu 22.04 as that seems to work on all platforms. I intermittently had the _NOT_BUILT issue, but I think that was due to not giving CMake enough time to configure after first open.

There are a few FIXME comments open in this PR, both relating to vcpkg. I needed to set VCPKG_FORCE_SYSTEM_BINARIES to run on MacOS and had to add a new arm64-linux.cmake triplet. I'm not sure if these are idiomatic, any feedback is welcome.

- Installs python, git, gdb for dev environment
- Python venv created in /tools/venv (in container, not on local machine) with build requirements pre-installed.
- vcpkg cloned and setup in /tools/vcpkg. Note the FIXME on VCPKG_FORCE_SYSTEM_BINARIES added to get working on MacOS running a Linux container, as it may need to be made conditional on platform.
- PYTHONPATH and PATH setup so integrated terminal works.
- First draft of developer guide README.md.
- devcontainer.json has cmake and python settings as in Dockerfile
- When running a Linux container on MacOS, the mpg123 library fails with REAL_IS_FIXED, which I think means that this distro/arch combination fails to infer it automatically. As a workaround I explicitly set HAVE_FPU. If this is OK, I can remove the FIXME comment, but it's there for now for discussion in PR.
- Contains various python and C++ debug scenarios.
- Copy to .vscode/launch.json to use in development.
- See .devcontainer/README.md for full information.
- These flags are automatically set via `CMakePresets.json`.
- Added notes on the need to wait while CMake is configuring. This needs to finish for tests to be available.
@alexallmont alexallmont linked an issue Nov 20, 2024 that may be closed by this pull request
@inakleinbottle
Copy link
Contributor

VCPKG_FORCE_SYSTEM_BINARIES almost surely is not a good idea in general, but if that's what is needed on this build than it's probably fine.

Comment on lines +12 to +22
RUN apt-get install -y \
git \
gdb \
curl \
zip \
unzip \
tar \
autoconf \
libtool \
libtool-bin \
pkg-config
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not an issue, but I believe the build-essential metapackage includes git, gdb, gcc, autconf, libtool, automake, and pkgconfig. It's sometimes handy if you need to set up the environment without needing to remember all the individual packages.

I don't know if you also need automake. Presumably not if the builds are running.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the GitHub CI images like ubuntu-latest these tools are present, but for the regular container I had to add them manually. It seems that the default ubuntu image is very minimal (no git for example).

pytest-cov

# FIXME make conditional on arm, s390x, ppc64le and riscv
ENV VCPKG_FORCE_SYSTEM_BINARIES=1
Copy link
Contributor

Choose a reason for hiding this comment

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

Upon checking the documentation, this just forces VCPKG not to install things like CMake and Ninja. This probably doesn't hurt anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, sorry, I saw your last reply after I'd made a change! I've made it conditional so only sets this when uname -m is aarch64. I'll leave it in though, as I would prefer not to set build flags unnecessarily; now if one is working on a Linux container they can be sure no extra flags are being set.

# Install vcpkg in checkout location
RUN git clone https://github.com/Microsoft/vcpkg.git /tools/vcpkg
RUN /tools/vcpkg/bootstrap-vcpkg.sh
ENV PATH /tools/vcpkg:$PATH
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you need to add vcpkg too the PATH. Since RoughPy uses a manifest build, we never actually interact with the binary itself directly. Instead, you should set the VCPKG_ROOT environment variable (RoughPy's configuration will detect this), or set the CMAKE_TOOLCHAIN_FILE environment variable to ${VCPKG_ROOT}/scripts/buildsystems/vcpkg.cmake, or otherwise specify this on the command line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Sam. Setting VCPKG_ROOT works perfectly. Double-checked on fresh clone and it's building everything from scratch correctly.

Copy link
Contributor

@inakleinbottle inakleinbottle 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 to me.

- It seems that without setting ENV, that vcpkg does not always pick up this variable and setup can fail. Committing here to test on a fresh clone.
- Replace FIXME with clearer comment. So far this setting is only required when building containerized on MacOS, presumably because mgp123 wants specific instructions for cross-compilation or embedded work. Here it is fine to set this, as the Linux-on-MacOS target platform will definitely have an FPU.
@alexallmont alexallmont merged commit 01dbf41 into main Nov 21, 2024
3 checks passed
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.

Add devcontainer
2 participants