tox-dev / tox-uv

Use https://github.com/astral-sh/uv with tox
MIT License
104 stars 17 forks source link

uv-venv-lock-runner silently ignores skip-install/deps #92

Closed hynek closed 1 month ago

hynek commented 1 month ago

First off, the uv.lock support seems to be mostly working, so great work! I've ran into a few edge cases that either need documentation or fixes.

This is the first one: I know this is kinda nonsense, but if you set

[testenv]
runner = uv-venv-lock-runner

it's automatically inherited and I've got a target

[testenv:pre-commit]
skip_install = true
deps = pre-commit
commands = pre-commit run --all-files

which fails with:

$ uvx --with tox-uv tox -re pre-commit
pre-commit: remove tox env folder /Users/hynek/FOSS/attrs/.tox/pre-commit
pre-commit: venv> /Users/hynek/Library/Caches/uv/archive-v0/u1hhc39Y_1D6r8mvsDrOq/bin/uv venv -p /Users/hynek/Library/Caches/uv/archive-v0/u1hhc39Y_1D6r8mvsDrOq/bin/python --allow-existing /Users/hynek/FOSS/attrs/.tox/pre-commit
pre-commit: uv-sync> uv sync --frozen
pre-commit: commands[0]> pre-commit run --all-files
pre-commit: failed with pre-commit is not allowed, use allowlist_externals to allow it

IOW it silently ignores the skip_install and deps options.

If you don't want to support skip_install/deps with uv-venv-lock-runner (which is totally fair enough!) there should be more explicit error messages + docs, I think?

gaborbernat commented 1 month ago

Yeah, we definitely don't want to support it. As far as warning goes, I don't know. That can get tricky because you can get caught up on inheritance again and warn just because someone specified the deps at the base level. This is the reason why I didn't want to enable this feature just by the presence of the lock file. I'm open to better documentation, but I'm not sure how, but feel free to fill in a pull request.

hynek commented 1 month ago

I don't care deeply about it, but FWIW, you can support skip_install by simply passing --no-install-project to uv sync.

hynek commented 1 month ago

Experimenting some more, I think it would be really nice if we could have uv sync --no-install-project to install dependencies and then use --install-pkg via uv pip install to install pre-built wheels into it.

Currently we have the choice between locked environments and better packaging verification. That seems an unnecessary dichotomy. 🤔

gaborbernat commented 1 month ago

I can't say that I agree with you now, but I will give it a thought.

If you can provide more detailed explanation of how this feature would be used and why it would be really nice improvement over the other namespace, I'm open to listen to it.

hynek commented 1 month ago

What do you mean by namespace here, exactly? But let me show you what I do in most of my projects.

First, I build the wheels for it:

  build-package:
    name: Build & verify package
    runs-on: ubuntu-latest

    steps:
      - uses: actions/checkout@v4
        with:
          fetch-depth: 0

      - uses: hynek/build-and-inspect-python-package@v2
        id: baipp

    outputs:
      # Used to define the matrix for tests below. The value is based on
      # packaging metadata (trove classifiers).
      supported-python-versions: ${{ steps.baipp.outputs.supported_python_classifiers_json_array }}

All test jobs depend on this step.

And then I use that wheel to run my tests against it:

tests:
    name: Tests on ${{ matrix.python-version }}
    runs-on: ubuntu-latest
    needs: build-package

    strategy:
      fail-fast: false
      matrix:
        # Created by the build-and-inspect-python-package action above.
        python-version: ${{ fromJson(needs.build-package.outputs.supported-python-versions) }}

    steps:
      - name: Download pre-built packages
        uses: actions/download-artifact@v4
        with:
          name: Packages
          path: dist
      - run: tar xf dist/*.tar.gz --strip-components=1
      - uses: actions/setup-python@v5
        with:
          python-version: ${{ matrix.python-version }}
          allow-prereleases: true
      - uses: hynek/setup-cached-uv@v2

      - name: Remove src to ensure tests run against wheel
        run: rm -rf src   # <======

      - run: >
          uvx --with=tox-uv
          tox run
          --installpkg dist/*.whl  # <======
          -e py$(echo ${{ matrix.python-version }} | tr -d .)

I really just want a stable tests environments that I install my stuff under test into -- that's why I care about uv.lock.

Doesn't also tox have the concept of preparing venvs without installing the app/pkg into it? Same concept, no?

BTW here's my experiment to follow along: https://github.com/python-attrs/attrs/pull/1349

gaborbernat commented 1 month ago

@hynek so if I understand this correctly, what you want is that if the --installpkg is present uv sync to run with --no-install-project and then we install the wheel via uv pip install. Yes?

hynek commented 1 month ago

Yes! 🎯

gaborbernat commented 1 month ago

That way you can still test locally via uv sync with install project and in the CI separate.

gaborbernat commented 1 month ago

@hynek there you go https://github.com/tox-dev/tox-uv/releases/tag/1.13.0

hynek commented 1 month ago

awesome thanks! I don't have time today to look closer, but https://github.com/python-attrs/attrs/actions/runs/10935175699/job/30386034044 looks nice and green!

gaborbernat commented 1 month ago
hynek commented 1 month ago

Great, now I have to rewrite half of my next video. >.<

gaborbernat commented 1 month ago

image

Should be quick 😆