Skip to content

Conversation

gstrauss
Copy link
Contributor

@gstrauss gstrauss commented Apr 2, 2025

  • use pwsh (instead of powershell)
  • add work-dir option (e.g. to allow specifying 'D:' instead of default 'C:')

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.

@gstrauss

This comment was marked as resolved.

@gstrauss
Copy link
Contributor Author

gstrauss commented Apr 2, 2025

Wow. The change from powershell and pwsh is causing the tests to fail. (expletive deleted)

@gstrauss

This comment was marked as resolved.

@gstrauss
Copy link
Contributor Author

gstrauss commented Apr 3, 2025

@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 pwsh instead of powershell. I still wonder why some of the cygwin tests fail under pwsh.

@gstrauss
Copy link
Contributor Author

gstrauss commented Apr 4, 2025

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.

@gstrauss
Copy link
Contributor Author

gstrauss commented Apr 4, 2025

@jon-turney Ready for review. Please review the two commits separately.

@gstrauss
Copy link
Contributor Author

gstrauss commented Apr 6, 2025

@jon-turney given the extreme performance difference between D:\ and C:\ for install-dir, what would you think about making the default install-dir D:\cygwin and, when install-dir is not specified, creating a symlink from C:\cygwin -> D:\cygwin for compatibility with existing CI .yml which use the default install-dir (C:\cygwin) ?

@jon-turney
Copy link
Member

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.

There's no way that's true.... (does some web searches)... it's true 🤦

@jon-turney
Copy link
Member

Please review the two commits separately.

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?

@jon-turney
Copy link
Member

jon-turney commented Apr 6, 2025

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?
(ii) Is there enough of a guarantee about how much free space there is on D:\ ?

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

given the extreme performance difference between D:\ and C:\ for install-dir, what would you think about making the default install-dir D:\cygwin and, when install-dir is not specified, creating a symlink from C:\cygwin -> D:\cygwin for compatibility with existing CI .yml which use the default install-dir (C:\cygwin) ?

Yeah, D:\ being the default seems like the sensible thing to do.

@gstrauss
Copy link
Contributor Author

gstrauss commented Apr 7, 2025

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?

The commit to switch from powershell to pwsh started with experiments in #25. Still, I think it is a good idea to move to the "better-supported" version of powershell from Microsoft, where "better" and "supported" are both highly up to interpretation because this is after all a Microsoft product.

@gstrauss
Copy link
Contributor Author

gstrauss commented Apr 7, 2025

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? (ii) Is there enough of a guarantee about how much free space there is on D:\ ?

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

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.

given the extreme performance difference between D:\ and C:\ for install-dir, what would you think about making the default install-dir D:\cygwin and, when install-dir is not specified, creating a symlink from C:\cygwin -> D:\cygwin for compatibility with existing CI .yml which use the default install-dir (C:\cygwin) ?

Yeah, D:\ being the default seems like the sensible thing to do.

I'll sketch this out later tonight and will update the PR.

@gstrauss
Copy link
Contributor Author

gstrauss commented Apr 7, 2025

Get-Volume run from the github runner returns:



DriveLetter FriendlyName      FileSystemType DriveType HealthStatus OperationalStatus SizeRemaining      Size
----------- ------------      -------------- --------- ------------ ----------------- -------------      ----
D           Temporary Storage NTFS           Fixed     Healthy      OK                    147.02 GB    150 GB
A                             Unknown        Removable Healthy      Unknown                     0 B       0 B
            System Reserved   NTFS           Fixed     Healthy      OK                    464.96 MB    500 MB
C           Windows           NTFS           Fixed     Healthy      OK                      87.4 GB 255.51 GB

We should clearly be using D:\ on github. There is plenty of space and it is intended as temporary storage.
Do we have to worry about someone using this action somewhere else?

@gstrauss
Copy link
Contributor Author

gstrauss commented Apr 7, 2025

Changing the default to D:\ for work-dir and the cygwin-install-action CI now takes 1 min 6 sec instead of 1 min 33 sec. For lighttpd CI, using the D:\ drive saves about 2 1/2 min to install to D:\cygwin and then compile and test lighttpd (down from 10 min to about 7 1/2 min total).

@gstrauss
Copy link
Contributor Author

gstrauss commented Apr 7, 2025

I added a test for D:\ and fall back to C:\ if D:\ does not exist.

@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 install-dir) use of cygwin-install-action noticeably faster.
#26 should make runs more reliable. I'm interested in getting both of these merged.

@jon-turney
Copy link
Member

Do we have to worry about someone using this action somewhere else?

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.

@jon-turney
Copy link
Member

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:

  • use $true/$false rather than literals
  • powershell -> pwsh
  • use ternary assignment operator where possible

@gstrauss
Copy link
Contributor Author

gstrauss commented Apr 8, 2025

Thanks!

Now that you mention it, I thought the ternary operator required pwsh.

use $true/$false rather than literals

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.

(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)

The cygwin-install-action CI did not work when work-dir was set until I added the C:\cygwin -> D:\cygwin symlink, so that was a good before and after test. What did work was my lighttpd CI for cygwin, since when I specified work-dir, I also changed the path used when invoking the shell. Yes, I did do some testing. 😄

I'll rebase the PR on top of your changes shortly.

@gstrauss
Copy link
Contributor Author

gstrauss commented Apr 8, 2025

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.

@lazka
Copy link

lazka commented Apr 11, 2025

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.

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.

vszakats added a commit to curl/curl that referenced this pull request Apr 12, 2025
- 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
nbaws pushed a commit to nbaws/curl that referenced this pull request Apr 26, 2025
- 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
nbaws pushed a commit to nbaws/curl that referenced this pull request Apr 26, 2025
- 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
gstrauss added 2 commits June 2, 2025 00:04
shell: pwsh        # uses Powershell 7 (latest)
shell: powershell  # uses Powershell 5 (stuck at Powershell 5)

Signed-off-by: Glenn Strauss <gstrauss@gluelogic.com>
Signed-off-by: Glenn Strauss <gstrauss@gluelogic.com>
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.

3 participants