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

refactor: use virtual environment to be consistent #560

Merged
merged 23 commits into from
Sep 24, 2024

Conversation

SMoraisAnsys
Copy link
Contributor

@SMoraisAnsys SMoraisAnsys commented Sep 6, 2024

Following the work on #543, it seems that there was a need for three things:

  • better handling of unexpected behavior when working with VMs, see Errors when working in a VM #559;
  • better coherence in how we deal with poetry and pip build backends;
  • better handling of the corner case where poetry and the project have shared dependencies with different versions.

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 use python -m pip install . while now we detect the build backend and use poetry if appropriate.

Note: following @Andy-Grigg comments, we use a "hack" to ensure that poetry and the project are not installed in the same environment and thus don't clash with each other if they share common dependencies with different version. The idea is to use pipx to install poetry in a temp folder cleaned up before and after running each job. The only side effect is for VMs as pipx will still be here afterward.

@SMoraisAnsys SMoraisAnsys requested a review from a team as a code owner September 6, 2024 14:21
@ansys-reviewer-bot
Copy link
Contributor

Thanks for opening a Pull Request. If you want to perform a review write a comment saying:

@ansys-reviewer-bot review

@github-actions github-actions bot added the bug Defects or glitches reported by users or developers label Sep 6, 2024
Copy link
Member

@RobPasMue RobPasMue left a 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

_doc-build-windows/action.yml Outdated Show resolved Hide resolved
check-vulnerabilities/action.yml Outdated Show resolved Hide resolved
code-style/action.yml Outdated Show resolved Hide resolved
tests-pytest/action.yml Outdated Show resolved Hide resolved
_doc-build-linux/action.yml Outdated Show resolved Hide resolved
_doc-build-windows/action.yml Outdated Show resolved Hide resolved
@SMoraisAnsys SMoraisAnsys marked this pull request as draft September 9, 2024 09:13
doc-build/action.yml Outdated Show resolved Hide resolved
doc-build/action.yml Outdated Show resolved Hide resolved
tests-pytest/action.yml Outdated Show resolved Hide resolved
@SMoraisAnsys SMoraisAnsys marked this pull request as ready for review September 19, 2024 11:58
@RobPasMue
Copy link
Member

You sure had some fun in this PR @SMoraisAnsys 😄

Copy link
Member

@RobPasMue RobPasMue left a 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

_doc-build-linux/action.yml Show resolved Hide resolved
_doc-build-linux/action.yml Outdated Show resolved Hide resolved
Copy link

@Andy-Grigg Andy-Grigg left a 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?

_doc-build-linux/action.yml Outdated Show resolved Hide resolved
_doc-build-windows/action.yml Outdated Show resolved Hide resolved
Co-authored-by: Roberto Pastor Muela <[email protected]>
Copy link

@Andy-Grigg Andy-Grigg left a 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!

@SMoraisAnsys
Copy link
Contributor Author

The changes look good to me. Thanks!

I need to double check with some testing though :) Thanks again for the feedback !

@SMoraisAnsys
Copy link
Contributor Author

Test closing this PR due to workflow problem

@SMoraisAnsys SMoraisAnsys marked this pull request as ready for review September 24, 2024 09:58
Copy link
Member

@jorgepiloto jorgepiloto left a 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.

@jorgepiloto jorgepiloto added this to the v8 milestone Sep 24, 2024
@SMoraisAnsys SMoraisAnsys merged commit 45bb889 into main Sep 24, 2024
16 checks passed
@SMoraisAnsys SMoraisAnsys deleted the fix/inconsistent-behavior branch September 24, 2024 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Defects or glitches reported by users or developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants