-
Notifications
You must be signed in to change notification settings - Fork 7
feat(actions): expose python-path output and propagate it to build-wheelhouse #1038
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
base: main
Are you sure you want to change the base?
Conversation
…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.
| the virtual environment, such as running smoke tests. | ||
| value: ${{ steps.virtual-environment-activation-command.outputs.ACTIVATE_VENV }} | ||
|
|
||
| python-path: |
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.
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.
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.
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!
…p to emit PYTHON_PATH
…yansys/actions into feat/adding-python-path-output
jorgepiloto
left a comment
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 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.
|
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 |
|
Why not activate the venv and run python normally then? |
|
Sorry but I find it difficult to grasp the added value associated with these changes. Since we already have the |
|
Yeah .. I'd recommend to just avoid this. Please use |
I believe it makes more sense to export the
pythonpath, so it can be used as an env var:Potentially you could do (I will need a double check):
Or maybe we can do something like:
Changes