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

fix(venv): respect base_python passed as absolute path #69

Merged
merged 5 commits into from
Jul 26, 2024

Conversation

paveldikov
Copy link
Contributor

Fixes: tox-dev/tox#3191


© 2024 Morgan Stanley.

THIS SOFTWARE IS CONTRIBUTED SUBJECT TO THE TERMS OF THE APACHE LICENSE V.2.0.

THIS SOFTWARE IS LICENSED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE AND ANY WARRANTY OF NON-INFRINGEMENT, ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. THIS SOFTWARE MAY BE REDISTRIBUTED TO OTHERS ONLY BY EFFECTIVELY USING THIS OR ANOTHER EQUIVALENT DISCLAIMER IN ADDITION TO ANY OTHER REQUIRED LICENSE TERMS.

src/tox_uv/_venv.py Outdated Show resolved Hide resolved
Copy link
Member

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

My change request has not been addressed.

@paveldikov paveldikov force-pushed the fix/tox-3191 branch 2 times, most recently from 6de4439 to d14f56b Compare July 21, 2024 10:08
@paveldikov paveldikov requested a review from gaborbernat July 21, 2024 10:10
@paveldikov paveldikov force-pushed the fix/tox-3191 branch 2 times, most recently from de2e669 to 503fefa Compare July 21, 2024 12:02
@paveldikov
Copy link
Contributor Author

@gaborbernat bumping just in case this got lost in transit. I've edited my change to call out to interpreter to mine info for PythonInfo tuple. Can you please re-review?

Copy link
Member

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

We should have a test for every conflict type.

tests/test_tox_uv_venv.py Outdated Show resolved Hide resolved
src/tox_uv/_venv.py Outdated Show resolved Hide resolved
src/tox_uv/_venv.py Outdated Show resolved Hide resolved
src/tox_uv/_venv.py Outdated Show resolved Hide resolved
src/tox_uv/_venv.py Outdated Show resolved Hide resolved
gaborbernat
gaborbernat previously approved these changes Jul 24, 2024
@gaborbernat gaborbernat enabled auto-merge (squash) July 24, 2024 13:15
auto-merge was automatically disabled July 24, 2024 13:27

Head branch was pushed to by a user without write access

gaborbernat
gaborbernat previously approved these changes Jul 24, 2024
@gaborbernat gaborbernat enabled auto-merge (squash) July 24, 2024 13:31
@paveldikov
Copy link
Contributor Author

Thanks for the approval! A few conversations are still unresolved (I think they're pretty unambiguous, but I will let you decide)

@gaborbernat
Copy link
Member

CI not green 😁

auto-merge was automatically disabled July 24, 2024 14:41

Head branch was pushed to by a user without write access

@paveldikov paveldikov requested a review from gaborbernat July 24, 2024 14:42
@paveldikov
Copy link
Contributor Author

D'oh! This caught an actual bug -- would have been impossible to specify an alternate Python installation of the same major/minor version.

I'm doing these fixes on my phone so apologies for the back-and-forth (I can't run the test suite locally). If it keeps failing I'll have a 'proper' look at it tonight

gaborbernat
gaborbernat previously approved these changes Jul 24, 2024
Copy link
Member

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

@gaborbernat
Copy link
Member

check / test 3.8 on windows-latest (pull_request) Failing after 1m ...

@paveldikov
Copy link
Contributor Author

paveldikov commented Jul 24, 2024

check / test 3.8 on windows-latest (pull_request) Failing after 1m ...

Amended. Hoping that's it...

P.S. perhaps too little too late for this one, but is there a way I can trigger CI on my own? Would have spared us a lot of the to'ing and fro'ing :-P

@paveldikov paveldikov requested a review from gaborbernat July 24, 2024 18:36
@paveldikov
Copy link
Contributor Author

@gaborbernat bumping :-)

Copy link
Member

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

@gaborbernat gaborbernat merged commit 0f5cfa4 into tox-dev:main Jul 26, 2024
22 checks passed
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.

base_python with full path fails with env name conflict
2 participants