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

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

Closed
hynek opened this issue Sep 19, 2024 · 13 comments · Fixed by #96
Closed

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

hynek opened this issue Sep 19, 2024 · 13 comments · Fixed by #96
Labels
bug Something isn't working

Comments

@hynek
Copy link
Contributor

hynek commented Sep 19, 2024

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?

@hynek hynek added the bug Something isn't working label Sep 19, 2024
@hynek hynek changed the title uv-venv-lock-runner ignores skip-install/deps uv-venv-lock-runner silently ignores skip-install/deps Sep 19, 2024
@gaborbernat
Copy link
Member

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
Copy link
Contributor Author

hynek commented Sep 19, 2024

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

@hynek
Copy link
Contributor Author

hynek commented Sep 19, 2024

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
Copy link
Member

gaborbernat commented Sep 19, 2024

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
Copy link
Contributor Author

hynek commented Sep 19, 2024

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: python-attrs/attrs#1349

@gaborbernat
Copy link
Member

@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
Copy link
Contributor Author

hynek commented Sep 19, 2024

Yes! 🎯

@gaborbernat
Copy link
Member

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

@gaborbernat
Copy link
Member

@hynek
Copy link
Contributor Author

hynek commented Sep 19, 2024

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
Copy link
Member

@hynek
Copy link
Contributor Author

hynek commented Sep 19, 2024

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

@gaborbernat
Copy link
Member

image

Should be quick 😆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants