fix: if miktex is available then make use of it#1305
Conversation
AlejandroFernandezLuces
left a comment
There was a problem hiding this comment.
LGTM if it fixes the issue. Just an opinionated suggestion. Is it possible to handle the ifs logic with github workflow syntax and keep powershell minimal? I really hate powershell it is so hard to read.
I'm thinking of something like: try to use user installation. If it fails, proceed to next approach.
We will hardly get around the powershell syntax - especially for the Windows-based action 😄 . I agree with you, it's not my favourite either... but even if we split it into GitHub workflow steps, we will still need to verify the exit codes from previous powershell operations, store paths into the env, etc. -- meaning that we might be able to reduce 20% of the PowerShell syntax at the expense of increasing a lot the number of steps ( I envision at least 3 new workflow steps in that case)... So, overall, the gain is not too much 😄 Let's discuss this altogether in the next team meeting (getting rid of PowerShell syntax as much as possible) - but if we attempt this, it will have to be done at a larger scale. And we would have to ensure that all runners have a bash terminal (Windows runners included, since sometimes the self-hosted ones might be missing it). But I'm all for it! |
MaxJPRey
left a comment
There was a problem hiding this comment.
Great! I was rusty with PowerShell though.
…ub.com/ansys/actions into fix/use-miktex-if-available-on-windows
SMoraisAnsys
left a comment
There was a problem hiding this comment.
LGTM, thanks for handling that @RobPasMue. I left a minor comment to remove the extra argument (and had a great time discovering coding in powershell... \o/)
Given the problem that happened in the past with choco and potential future updates, I would highly recommend any installation at system level to be removed and reinstalled at user level but if this. But, that's another siubject :)
Co-authored-by: Sébastien Morais <146729917+SMoraisAnsys@users.noreply.github.com>
Let's leave that for another day 😄 - I think that's a broader discussion we should have for all choco-related installs we perform. And we should handle the changes all at the same time throughout our actions |
|
@MaxJPRey @SMoraisAnsys @klmcadams - since your comments implied modifications, please review the PR again before we proceed to merge |
SMoraisAnsys
left a comment
There was a problem hiding this comment.
LGTM, just revert the pinned branch and all good :)
|
@moe-ad - could you please trigger a patch release with these changes? |
Co-authored-by: pyansys-ci-bot <92810346+pyansys-ci-bot@users.noreply.github.com> Co-authored-by: Sébastien Morais <146729917+SMoraisAnsys@users.noreply.github.com>
|
Release completed. |
|
Thanks @moe-ad !! |
Reported in ansys/pysherlock#778
If MikTex is installed already at system level, the new action implementation causes it to fail since we simply take it from the local installation. Let's make it more "dynamic"