Skip to content

Conversation

@germa89
Copy link
Contributor

@germa89 germa89 commented Oct 20, 2025

I believe it makes more sense to export the python path, so it can be used as an env var:

- name: "Build wheelhouse"
  uses: ansys/actions/build-wheelhouse@main
  with:
    library-name: ${{ env.LIBRARY_NAME }}
    operating-system: ${{ matrix.os }}
    python-version: ${{ env.MAIN_PYTHON_VERSION }}
    working-directory: .ci/${{ env.LIBRARY_NAME }}
    attest-provenance: true

- name: "do something..."
  env:
    PYTHON: ${{ steps.build-wheelhouse.output.python-path }}
  run:
    python something.py

Potentially you could do (I will need a double check):

- name: "do something..."
  shell: python
  env:
    PYTHON: ${{ steps.build-wheelhouse.output.python-path }}
  run:
    # my python code
    from ansys.mapdl.core import launch_mapdl

Or maybe we can do something like:

- name: "do something..."
  shell: ${{ steps.build-wheelhouse.output.python-path }} {0}
  run:
    # my python code
    from ansys.mapdl.core import launch_mapdl

Changes

  • Add python-path output and a "Get Python path" step to the _setup-python composite action.
  • Add id: setup-python to the build-wheelhouse setup step and expose python-path via steps.setup-python.outputs.python-path.

…eelhouse

- Add python-path output and a "Get Python path" step to the _setup-python composite action.
- Add id: setup-python to the build-wheelhouse setup step and expose python-path via steps.setup-python.outputs.python-path.
@germa89 germa89 requested a review from a team as a code owner October 20, 2025 11:16
@github-actions github-actions bot added the enhancement General improvements to existing features label Oct 20, 2025
the virtual environment, such as running smoke tests.
value: ${{ steps.virtual-environment-activation-command.outputs.ACTIVATE_VENV }}

python-path:
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't you be retrieving the virtual env instead of the python installation ? If yes, I think that it would make more sense to expose steps.virtual-environment-activation-command.outputs.ACTIVATE_VENV instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @SMoraisAnsys

You are right, I was retrieving the python installation. The idea should be to retrieve the venv python executable. I fixed that now.

I still think it is more interesting to retrieve the executable path than the activation command, for the reasons I mentioned in the PR (I updated it).

Please feel free to review!

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.

The actions were designed to be unique jobs. I have the feeling that the more flexibility and options we add, the more challenging it is to maintain them.

@RobPasMue
Copy link
Member

RobPasMue commented Oct 20, 2025

I fully agree with @jorgepiloto on this one... also, we shouldn't be performing extra operations after the wheelhouse has been built

@germa89
Copy link
Contributor Author

germa89 commented Oct 20, 2025

I fully agree with @jorgepiloto on this one... also, we shouldn't be performing extra operations after the wheelhouse has been built

I agree... but I think we mentioned that we might need to run some custom smoke tests after the build. That is why the other output activate_venv was implemented. But that option (export commands to activate a venv) is a bit non-standard, hence I was proposing this solution.

@RobPasMue
Copy link
Member

Why not activate the venv and run python normally then?

@SMoraisAnsys
Copy link
Contributor

Sorry but I find it difficult to grasp the added value associated with these changes. Since we already have the activate-venv output, can't you just leverage that directly ?

@RobPasMue
Copy link
Member

Yeah .. I'd recommend to just avoid this. Please use activate_venv @germa89. Proposing to close this PR..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement General improvements to existing features

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants