-
Notifications
You must be signed in to change notification settings - Fork 5
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
refactor: use virtual environment to be consistent #560
Conversation
Thanks for opening a Pull Request. If you want to perform a review write a comment saying: @ansys-reviewer-bot review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM - just a minor comment
Co-authored-by: Maxime Rey <[email protected]>
You sure had some fun in this PR @SMoraisAnsys 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Some minor comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Poetry should be installed in a separate virtual environment to the project that's being tested/built, to avoid situations in which both poetry and the project have dependencies on different versions of the same package. I've included an approach that uses pipx to get around this.
@ludovicsteinbach I know you originally raised this topic about poetry support across all actions. Can you take a look as well please?
Co-authored-by: Roberto Pastor Muela <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good to me. Thanks!
I need to double check with some testing though :) Thanks again for the feedback ! |
Test closing this PR due to workflow problem |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the big effort here, @SMoraisAnsys. Approving. Just left one comment.
Following the work on #543, it seems that there was a need for three things:
poetry
andpip
build backends;This PR aims at tackling those three points by enforcing the use of a virtual environment, even when working with
poetry
.On top of those changes, the modified actions are now "more compatible" with
poetry
. Before, the default behavior was to usepython -m pip install .
while now we detect the build backend and usepoetry
if appropriate.