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

Make uv dependency optional #82

Closed
RomainBrault opened this issue Aug 29, 2024 · 8 comments
Closed

Make uv dependency optional #82

RomainBrault opened this issue Aug 29, 2024 · 8 comments
Labels
enhancement New feature or request

Comments

@RomainBrault
Copy link
Contributor

RomainBrault commented Aug 29, 2024

What's the problem this feature will solve?

Currently uv is installed in tox's virtualenv.

If a user have its own uv in another virutalenv, it might cause confusion and more management (e.g. updates)

Describe the solution you'd like

Follow PyPA's build implementation:

https://github.com/pypa/build/blob/main/src/build/env.py

        try:
            import uv

            self._uv_bin = uv.find_uv_bin()
        except (ModuleNotFoundError, FileNotFoundError):
            uv_bin = shutil.which('uv')
            if uv_bin is None:
                msg = 'uv executable not found'
                raise RuntimeError(msg) from None

            _ctx.log(f'Using external uv from {uv_bin}')
            self._uv_bin = uv_bin

In tox uv we could modify in https://github.com/tox-dev/tox-uv/blob/main/src/tox_uv/_venv.py:

    @property
    def uv(self) -> str:
        return find_uv_bin()

by

    @property
    def uv(self) -> str:
        try:
            import uv

            return uv.find_uv_bin()
        except (ModuleNotFoundError, FileNotFoundError):
            uv_bin = shutil.which('uv')
            if uv_bin is None:
                msg = 'uv executable not found'
                raise RuntimeError(msg) from None

        return uv_bin
 
        

Remove the toplevel import and make the dependency optional in pyproject.toml

Additional context

To me the difficult part here is the test / coverage. I don't really know how to do.

It will also require to add some documentation, e.g.

By default tox-uv doesn't ship with uv. If you don´t have uv installed on your system, you can either

  1. Let tox-uv managed uv with pip install tox-uv[uv
  2. Install uv

The drawback I'm seeing is that it might break systems where people didn't have uv install as a global tool and used only uv through tox-uv.

Alternative Solutions

Do not make the uv dependency optional but prioritize system uv over tox-uv's uv.

    @property
    def uv(self) -> str:
         uv_bin = shutil.which('uv')
         if uv_bin is None:
             return uv.find_uv_bin()

         return uv_bin        
@RomainBrault RomainBrault added the enhancement New feature or request label Aug 29, 2024
@RomainBrault
Copy link
Contributor Author

also a @cached_property instead of @Property might be relevant.

@gaborbernat
Copy link
Member

I am strongly against this. The sane behavior is to manage our version of UV; otherwise we have no idea whether what we get it compatible or not with the functionality we want to provide.

@razy69
Copy link

razy69 commented Nov 22, 2024

Hello,

It could be great to have an option for it. eg: manage_uv_version (bool)

Use case:

I use a tool version manager, like asdf.
In my repo, i pin the version of uv i want, eg: 0.5.4.

I have a Makefile which wrap my commands.

make setup: asdf install uv version specified in '.tool-versions' file, then create a venv using system uv, source it and install tox + tox-uv
make tests: source venv and execute tox -e tests, which create a tox env, install tests dependencies using uv and run them

I'm already ensuring compatibility of uv/tox-uv using asdf '.tool-versions' and i would like to avoid extra uv installation.

Thanks.

@gaborbernat
Copy link
Member

I will still say no, just because you can ensure compatibility that doesn't mean in general is true.

@razy69
Copy link

razy69 commented Nov 22, 2024

You are absolutely right, that's why i propose to add it as an option.

Default behavior still the same but allow users to disable it if needed/wanted..

@MatteoGisondi
Copy link

Allowing opt-in behavior does trump the ensure compatibility argument, and makes the tool more flexible.

@gaborbernat
Copy link
Member

If someones opts in and then fails to ensure compatbility and reports an error who will need to troubleshoot the issue? I am pretty sure the answer is me.

@gaborbernat
Copy link
Member

Therefore, I will accept such PRs only if the contributor signs up to be maintainer too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants