Skip to content

Conversation

ldionne
Copy link
Member

@ldionne ldionne commented Oct 14, 2025

This patch rewrites the project build configuration using pyproject.toml, which is now the de-facto standard in Python. This single configuration file supersedes setup.py, tox.ini, mypy.ini, etc.

I checked the contents of the generated package and made sure they matched exactly (that caught a few issues where I originally excluded some data files).

This patch is careful not to change the version of dependencies. In pyproject.toml, we specify minimum package versions for the tool to work, and we provide a requirements.txt file that pins down specific versions for use by end-users.

Currently, a few tests are failing if we don't pin exact versions when running them, so we're still pinning exact versions when we run tests via tox. The same holds for the type checker.

In the future, we could upgrade towards a proper lockfile instead of manually constructed requirements.txt files.

Fixes #77

@ldionne ldionne requested a review from DavidSpickett October 14, 2025 16:30
@ldionne ldionne force-pushed the review/toml-project branch 5 times, most recently from bde2993 to ec8fc6a Compare October 14, 2025 17:36
This patch rewrites the project build configuration using pyproject.toml,
which is now the de-facto standard in Python. This single configuration
file supersedes setup.py, tox.ini, mypy.ini, etc.

I checked the contents of the generated package and made sure they matched
exactly (that caught a few issues where I originally excluded some data
files).

This patch is careful not to change the version of dependencies. In
pyproject.toml, we specify minimum package versions for the tool to
work, and we provide a requirements.txt file that pins down specific
versions for use by end-users.

Currently, a few tests are failing if we don't pin exact versions when
running them, so we're still pinning exact versions when we run tests
via tox. The same holds for the type checker.

In the future, we could upgrade towards a proper lockfile instead of
manually constructed requirements.txt files.

Fixes llvm#77
@ldionne ldionne force-pushed the review/toml-project branch from acce046 to 07fddd3 Compare October 14, 2025 18:53
Copy link
Contributor

@DavidSpickett DavidSpickett left a comment

Choose a reason for hiding this comment

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

As far as I can tell, everything got moved over. Any improvements or corrections from the original information to be considered later.

Copy link
Contributor

@DavidSpickett DavidSpickett left a comment

Choose a reason for hiding this comment

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

LGTM, include the readme changes if you like.

Copy link

@DaftanoPro DaftanoPro left a comment

Choose a reason for hiding this comment

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

Good work: looks good as initial step.
Just a minor suggestion.
Some of those TODOs can be resolved with uv.

@ldionne
Copy link
Member Author

ldionne commented Oct 15, 2025

Good work: looks good as initial step. Just a minor suggestion. Some of those TODOs can be resolved with uv.

Thanks for the review! I think the TODOs are actually things where we need to fix our tests. For example, mypy doesn't work if you pull in the actual dependencies because it starts type checking more stuff, and that fails. So I think actual code changes will be required to unblock that.

@ldionne ldionne merged commit f0800db into llvm:main Oct 15, 2025
5 checks passed
@ldionne ldionne deleted the review/toml-project branch October 15, 2025 13:27
Comment on lines +7 to 15
COPY . /var/src/lnt

COPY requirements*.txt setup.py .
# setup.py uses lnt.__version__ etc.
COPY lnt/__init__.py lnt/__init__.py
# we build the cperf extension during install
COPY lnt/testing/profile lnt/testing/profile
WORKDIR /var/src/lnt

RUN pip3 install -r requirements.server.txt \
&& apk --purge del .build-deps \
&& mkdir /var/log/lnt

COPY . .
COPY docker/docker-entrypoint.sh docker/wait_db /usr/local/bin/
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did we remove the individual COPYs? This will cause unnecessary layers to be invalidated.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think if you just replace the original COPY requirements*.txt setup.py . with COPY requirements*.txt pyproject.toml . we should still be able to avoid reinstalling all the requirements on every build

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.

LNT should install using current "best practice" for Python

4 participants