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

Make the install directory configurable #3

Merged
merged 3 commits into from
Feb 1, 2022
Merged

Make the install directory configurable #3

merged 3 commits into from
Feb 1, 2022

Conversation

me-and
Copy link
Contributor

@me-and me-and commented Feb 1, 2022

Default to maintaining the current behaviour, but allow users of this
action to change the install directory. This means it's possible, for
example, to use the default-everywhere-else behaviour of installing
64-bit Cygwin to C:\cygwin64, or to ease migration from other GitHub
Actions such as egor-tensin/setup-cygwin, which defaults to installing
in C:\tools\cygwin.

@me-and
Copy link
Contributor Author

me-and commented Feb 1, 2022

I tried adding a test, too, but apparently the YAML is invalid:

The workflow is not valid. .github/workflows/test.yml (Line: 119, Col: 14): Unrecognized named-value: 'matrix'. Located at position 1 within expression: matrix.install-dir

I'm utterly failing to work out what the problem is, though – as best I can tell the bit it's objecting to is more-or-less identical to the similar example in "complex-test"…

@me-and
Copy link
Contributor Author

me-and commented Feb 1, 2022

Ah, fixed that test problem; I was mangling my Git commands unhelpfully.

Default to maintaining the current behaviour, but allow users of this
action to change the install directory.  This means it's possible, for
example, to use the default-everywhere-else behaviour of installing
64-bit Cygwin to C:\cygwin64, or to ease migration from other GitHub
Actions such as egor-tensin/setup-cygwin, which defaults to installing
in C:\tools\cygwin.
@me-and
Copy link
Contributor Author

me-and commented Feb 1, 2022

Okay, that was clearly not ready to be a PR at the time I created it, given just how many times I needed to force-push to correct the test code. But the test is now working on my fork, so I think this PR is actually ready to be looked at and pulled now.

@jon-turney
Copy link
Member

Thanks! This was just me being lazy since a fixed path was all I needed for my particular application.

This also needs an change to README.md to document the new parameter.

@me-and
Copy link
Contributor Author

me-and commented Feb 1, 2022

Okay, removed the extraneous shell declaration in the test script, and updated the README to document the new option.

@jon-turney jon-turney merged commit 6e7f439 into cygwin:master Feb 1, 2022
@me-and me-and deleted the variable-install-dir branch February 1, 2022 21:51
@jon-turney
Copy link
Member

Thanks!

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.

2 participants