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

ENH: Name and type command line options of Windows module PowerShell script #252

Merged
merged 1 commit into from
Feb 22, 2023

Conversation

SimonRit
Copy link

This PR aims at improving the management of arguments in windows-download-cache-and-build-module-wheels.ps1 following problems encountered in RTKConsortium/RTK#535, namely passing multiple cmake options on the command line. The difference in the behaviors of PowerShell and Python args calls for another scripting language in my opinion (see #250 discussion) but this PR is an improvement over #248 in terms of clarity and consistency with other platforms. It is not backward compatible with it but that is probably ok since it is a recent change. See https://github.com/RTKConsortium/RTK/actions/runs/4243933294/jobs/7377339692 and https://github.com/RTKConsortium/RTK/actions/runs/4243933347 for successful use.

@SimonRit
Copy link
Author

(The last force-push is only a slight change of the doc.)

Copy link
Contributor

@tbirdso tbirdso left a comment

Choose a reason for hiding this comment

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

Looking great, thank you for your work here @SimonRit ! One small comment on extra quotes, after that is resolved these changes look good to me.

scripts/windows-download-cache-and-build-module-wheels.ps1 Outdated Show resolved Hide resolved
@tbirdso tbirdso merged commit ee05fd4 into InsightSoftwareConsortium:master Feb 22, 2023
@SimonRit SimonRit deleted the PowershellArgs branch February 23, 2023 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants