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

build: add devcontainer setup #1725

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

havogt
Copy link
Contributor

@havogt havogt commented Nov 5, 2024

No description provided.

@havogt havogt requested a review from egparedes November 5, 2024 12:14
Copy link
Contributor

@egparedes egparedes left a comment

Choose a reason for hiding this comment

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

Just a couple of suggestions to use uv:

Comment on lines +3 to +4
&& export DEBIAN_FRONTEND=noninteractive && apt-get install -y libboost-dev \
&& apt-get clean && rm -rf /var/cache/apt/* && rm -rf /var/lib/apt/lists/* && rm -rf /tmp/*
Copy link
Contributor

@egparedes egparedes Nov 5, 2024

Choose a reason for hiding this comment

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

Can we also add some basic python tooling like pipx & uv? For example:

Suggested change
&& export DEBIAN_FRONTEND=noninteractive && apt-get install -y libboost-dev \
&& apt-get clean && rm -rf /var/cache/apt/* && rm -rf /var/lib/apt/lists/* && rm -rf /tmp/*
&& export DEBIAN_FRONTEND=noninteractive && apt-get install -y libboost-dev \
&& apt-get install pipx && ensurepath && pipx install uv \
&& apt-get clean && rm -rf /var/cache/apt/* && rm -rf /var/lib/apt/lists/* && rm -rf /tmp/*

(the pipx installation assumes the base image is a reasonably recent Ubuntu version: https://pipx.pypa.io/latest/installation/#on-linux)

Comment on lines +4 to +9
python -m venv .venv
source .venv/bin/activate
pip install --upgrade pip setuptools wheel
pip install -e .
pip install -r requirements-dev.txt
pip install -i https://test.pypi.org/simple/ atlas4py
Copy link
Contributor

Choose a reason for hiding this comment

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

If we install uv in the dockerfile then we can use it here which should be nicer and much faster:

Suggested change
python -m venv .venv
source .venv/bin/activate
pip install --upgrade pip setuptools wheel
pip install -e .
pip install -r requirements-dev.txt
pip install -i https://test.pypi.org/simple/ atlas4py
uv venv .venv
source .venv/bin/activate
uv pip install -r requirements-dev.txt
uv pip install -e .
uv pip install -i https://test.pypi.org/simple/ atlas4py

@havogt
Copy link
Contributor Author

havogt commented Nov 5, 2024

Should we just --user or --system install instead of venv? Since the devcontainer is anyway an isolated environment...

@egparedes
Copy link
Contributor

Should we just --user or --system install instead of venv? Since the devcontainer is anyway an isolated environment...

Yes, I agree. I don't think we need a venv inside the container

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.

2 participants