-
-
Notifications
You must be signed in to change notification settings - Fork 188
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
Test Windows CI #657
Test Windows CI #657
Conversation
Thanks a lot @mamantoha 👍 Could you please tell me why do we need the permissions for |
@sdogruyol I don't know why https://github.com/crystal-ameba/github-action requires write permissions |
@mamantoha that's already enabled, do I need to do anything else? |
@sdogruyol I don't understand why |
Just reran the workflow and it failed with the same error. Not sure what's wrong with permissions 🤷 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great to get Windows support merged.
On the code side the only relevant change is Process.on_interrupt
- and that is direly needed. Maybe this could be merged on its own if CI needs more time?
- name: Check formatting | ||
run: crystal tool format --check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fails on Windows, not sure why. It works fine locally.
I would suggest to disable this spec on windows. Actually, it only needs to execute once - the expected result is the same on any runner (if: matrix.os == 'ubuntu-latest' && matrix.crystal == 'nightly'
)
check_ameba: | ||
runs-on: ubuntu-latest | ||
steps: | ||
- name: Install Crystal | ||
uses: crystal-lang/install-crystal@v1 | ||
- name: Check out repository code | ||
uses: actions/checkout@v3 | ||
- name: Crystal Ameba Linter | ||
id: crystal-ameba | ||
uses: crystal-ameba/github-action@v0.7.1 | ||
env: | ||
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The action seems to be broken (ref https://github.com/kemalcr/kemal/actions/runs/5443532334/jobs/9900164974?pr=657). So better exclude that from this PR. It's not necesary for Windows support.
The kemal/src/kemal/static_file_handler.cr Lines 30 to 38 in c995a2a
Both |
|
That part in stdlib is: request_path = Path.posix(request_path)
expanded_path = request_path.expand("/")
file_path = @public_dir.join(expanded_path.to_kind(Path::Kind.native)) |
Hi @HertzDevil @straight-shoota I don't have a Windows environment to test this build locally. Can someone else take over and continue working on this pull request? Thank you. |
shard.yml
Outdated
|
||
development_dependencies: | ||
ameba: | ||
github: crystal-ameba/ameba | ||
version: ~> 1.4.0 | ||
branch: master |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be reverted, I believe.
Closed in favor of #690 |
This was just a test for Windows CI