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

Switch to use uv pip install #2957

Merged
merged 39 commits into from
Aug 21, 2024
Merged

Conversation

CoolCat467
Copy link
Member

This pull request follows up on #2956 and is attempting to switch the continuous integration system to use astral.sh' new tool uv.

@CoolCat467 CoolCat467 added the dependencies Pull requests that update a dependency file label Feb 16, 2024
@CoolCat467 CoolCat467 marked this pull request as draft February 16, 2024 02:52
Copy link

codecov bot commented Feb 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.61%. Comparing base (fbb9d50) to head (3933ce2).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2957      +/-   ##
==========================================
- Coverage   99.63%   99.61%   -0.02%     
==========================================
  Files         121      121              
  Lines       17841    17841              
  Branches     3206     3206              
==========================================
- Hits        17776    17773       -3     
- Misses         46       48       +2     
- Partials       19       20       +1     

see 2 files with indirect coverage changes

@A5rocks
Copy link
Contributor

A5rocks commented Feb 16, 2024

This seems nontrivial, can we break this into 2 prs?

  1. replace pip-compile
  2. replace pip install

Personally I see (1) as much more valuable-- (2) doesn't change much. Personally I'm more excited by uv's ability to get the lowest compatible version so we can add a new CI run than by it replacing pip install.

@CoolCat467 CoolCat467 changed the title Switch to use uv. Closes #2956 Switch to use uv pip install Feb 17, 2024
ci.sh Outdated Show resolved Hide resolved
@CoolCat467 CoolCat467 requested a review from jakkdl June 27, 2024 18:33
ci.sh Outdated Show resolved Hide resolved
ci.sh Outdated Show resolved Hide resolved
ci.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@A5rocks A5rocks left a comment

Choose a reason for hiding this comment

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

Some of your changes remind me that we should probably run shellcheck over our scripts...

ci.sh Outdated

python -m build
python -m pip install dist/*.whl
wheel_package=$(ls dist/*.whl)
python -m uv pip install "trio @ $wheel_package"
Copy link
Contributor

Choose a reason for hiding this comment

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

We can probably lead the path with ./ to get this to work like before: astral-sh/uv#6052

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a particular reason not to leave it at my current solution?

Copy link
Contributor

Choose a reason for hiding this comment

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

Personal preference TBH. Maybe you disagree but I prefer moving logic out of shell scripts and into program arguments.

Feel free not to do it though.

@CoolCat467
Copy link
Member Author

Some of your changes remind me that we should probably run shellcheck over our scripts...

Problem is, it's written in haskell and the pre-commit integration for it uses docker, which is a mess

@A5rocks
Copy link
Contributor

A5rocks commented Aug 15, 2024

Some of your changes remind me that we should probably run shellcheck over our scripts...

Problem is, it's written in haskell and the pre-commit integration for it uses docker, which is a mess

Yeah I think it's fine not to check it in CI. It's not like our shell scripts are that important or change often; it's just a nicety.

@CoolCat467 CoolCat467 added the skip newsfragment Newsfragment is not required label Aug 21, 2024
@CoolCat467 CoolCat467 enabled auto-merge (squash) August 21, 2024 18:29
@CoolCat467 CoolCat467 merged commit e3cfb23 into python-trio:main Aug 21, 2024
36 of 37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file skip newsfragment Newsfragment is not required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants