-
Notifications
You must be signed in to change notification settings - Fork 13
use pwsh and add work-dir option #27
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: master
Are you sure you want to change the base?
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
Wow. The change from powershell and pwsh is causing the tests to fail. (expletive deleted) |
This comment was marked as resolved.
This comment was marked as resolved.
@jon-turney some quick (non-scientific n=2) tests running the lighttpd CI twice on C: took 4 min 12 sec, and 5 min 10 sec; and twice on D: took 2 min 24 sec, and 1 min 47 sec. While there is some slippage comparing times when then package download time from mirrors.kernel.org can vary, in my single-run test the C: drive took about 1 min 10 sec, and the D: drive took a few seconds less than that, so the rest of the time difference is likely the Windows bloatware "protecting" the C:\ drive (which contains C:\Windows) while setup.exe is extracting and installing the packages to the C: drive versus the D: drive. Running the same CI (once) with C:\setup.exe and C:\cygwin-packages, but having the install-dir D:\cygwin took 2 min 25 sec, so the existing install-dir option in the workflow appears to get most of the benefit to not being on C:. While using the D:\ drive for D:\setup.exe and D:\cygwin-packages is not worse than using C:\ for those as long as the installation directory is on D:\cygwin, I would understand if you think the install-dir option is sufficient for specifying D:\cygwin rather than the new work-vol option in this PR. In the lighttpd CI, I am using |
Turns out that powershell and pwsh did a fugly reimplementation of awk more than 40 years later after awk. You're more than welcome to suggest prettier syntax. The problem was that some of the basic string manipulation syntax changed between powershell (5) and pwsh (7) and the input strings were no longer being split under pwsh as they were under powershell. |
@jon-turney Ready for review. Please review the two commits separately. |
@jon-turney given the extreme performance difference between D:\ and C:\ for |
There's no way that's true.... (does some web searches)... it's true 🤦 |
Made a few minor comments of the packages dir. I don't have any comments on the "use pwsh" commit (my eyes glaze over at it's crufty syntax as well). But why is it here? Does the other commit depend on it in some way? |
Thanks for working on this! So, my concerns with all this are: (i) is there some reasonable indication that D:\ will exist and be faster on the VMs we run on? I found [1], which seems to answer (i), but couldn't find any details to answer (ii) [1] https://learn.microsoft.com/en-us/azure/virtual-machines/managed-disks-overview#temporary-disk
Yeah, D:\ being the default seems like the sensible thing to do. |
The commit to switch from |
On current github runners, the working copy of the source tree ends up on D:\a\ with git checkout, but I'll look to see if there are quick ways to test this and to fall back to C:\ if D:\ does not exist.
I'll sketch this out later tonight and will update the PR. |
We should clearly be using D:\ on github. There is plenty of space and it is intended as temporary storage. |
Changing the default to D:\ for |
I added a test for @jon-turney I believe I have addressed all of your concerns except for disk space checks on D:\ and "bullet-proofing", as you say, user-input sanity checks. As maintainer, you're welcome to edit this PR with any tweaks that you would like. This PR should make default (without specifying |
No, that wasn't what I was trying to say. Just that GitHub could change the specification and configuration of the VM whenever they feel like it. |
I've applied the work-vol changes, and added some simple tests of that option. (When writing the tests, I was kind of a confused how the tests passed before, without the C: -> D: symlink added, since C: is hardcoded in lots of places there) I kept the powershell -> pwsh patch separate out of some vague concern to minimize the amount of changes. List for me of things to look at later:
|
Thanks! Now that you mention it, I thought the ternary operator required pwsh.
I'm relatively new to pwsh, too, so please do check/verify my syntax. As I said, I am more than happy to take suggestions.
The cygwin-install-action CI did not work when I'll rebase the PR on top of your changes shortly. |
Rebased. The remaining commits are the change from powershell to pwsh, and using ternary operator. The commit using ternary operator is not important and can be omitted if you prefer. I removed the use of $true. I am not well-versed in pwsh on whether or not its use is recommended or preferred. |
One heuristic could be to default to "/" aka the current drive root. Each action step has "$GITHUB_WORKSPACE" set as the working directory, and it's likely that this is on a fast drive, even if they change things in the future. |
- to benefit from the new download retry mechanism. cygwin/cygwin-install-action#26 - to use a new setting that not only moves the Cygwin install target directory to the faster `D:` drive, but also the package download directory. Expecting a little performance improvement from this for the Cygwin install step. cygwin/cygwin-install-action@d3a7464 cygwin/cygwin-install-action#27 Closes #17040
- to benefit from the new download retry mechanism. cygwin/cygwin-install-action#26 - to use a new setting that not only moves the Cygwin install target directory to the faster `D:` drive, but also the package download directory. Expecting a little performance improvement from this for the Cygwin install step. cygwin/cygwin-install-action@d3a7464 cygwin/cygwin-install-action#27 Closes curl#17040
- to benefit from the new download retry mechanism. cygwin/cygwin-install-action#26 - to use a new setting that not only moves the Cygwin install target directory to the faster `D:` drive, but also the package download directory. Expecting a little performance improvement from this for the Cygwin install step. cygwin/cygwin-install-action@d3a7464 cygwin/cygwin-install-action#27 Closes curl#17040
shell: pwsh # uses Powershell 7 (latest) shell: powershell # uses Powershell 5 (stuck at Powershell 5) Signed-off-by: Glenn Strauss <[email protected]>
Signed-off-by: Glenn Strauss <[email protected]>
I find powershell to be fugly and touch it with a ten-foot-pole when I have to.
Suggestions for improvements to my syntax are welcome. Thanks.