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

Windows support MVP #690

Merged
merged 2 commits into from
Oct 1, 2024
Merged

Windows support MVP #690

merged 2 commits into from
Oct 1, 2024

Conversation

sdogruyol
Copy link
Member

This makes Kemal work natively on Windows 11. All specs pass and also Tested with my own Kemal project with 7K Crystal LOC Açık Türkiye

@mamantoha mamantoha mentioned this pull request Sep 30, 2024
src/kemal/config.cr Outdated Show resolved Hide resolved
src/kemal/static_file_handler.cr Show resolved Hide resolved
Comment on lines 25 to 29
{% if flag?(:windows) %}
client_response.body.strip.should eq("<html>Hello world\r\n</html>")
{% else %}
client_response.body.strip.should eq("<html>Hello world\n</html>")
{% end %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless the file is created on windows there should not be a difference in line endings.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tested this behaviour on a clone of Kemal on Windows and this change is needed for specs to pass.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps, yet it doesn't explain the difference.

Copy link
Contributor

@Blacksmoke16 Blacksmoke16 Sep 30, 2024

Choose a reason for hiding this comment

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

It might help adding https://github.com/athena-framework/athena/blob/master/.gitattributes to ensure line endings remain lf. Ran into the same problem with athena and iirc this helped a lot since before due to my git settings it was converting everything on checkout which was incorrect.

This might also solve the earlier content-length difference as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

I really think there should not be any difference in the output format depending on whether the spec runs on Windows or Linux.
This discrepancy is most likely caused by hello.ecr being checked out with CRLF line ending on Windows, when it should use LF. Configuring gitattributes should fix that (probably needs rules for *.cr and *.ecr).

@Sija
Copy link
Contributor

Sija commented Sep 30, 2024

StaticFileHandler also needs an update.

@sdogruyol
Copy link
Member Author

StaticFileHandler also needs an update.

It works with this change? What else needs to be updated?

@Sija
Copy link
Contributor

Sija commented Sep 30, 2024

Oh, maybe that was all what was needed. There are some other discrepancies between Kemal and Crystal implementations btw - not Windows related though, AFAIR.

@Sija
Copy link
Contributor

Sija commented Sep 30, 2024

CI support is missing to make sure the changes actually work.

@sdogruyol
Copy link
Member Author

CI support is missing to make sure the changes actually work.

Yeah. Haven't done any Windows CI config before, any help / guidance welcome 🙏

@Blacksmoke16
Copy link
Contributor

Should really just be adding windows-latest to https://github.com/kemalcr/kemal/blob/master/.github/workflows/ci.yml#L14. Worst case break out ameba and formatting checks to their own jobs if they don't like being ran on windows too. Also not sure why the tests are ran twice?

@sdogruyol
Copy link
Member Author

Thanks a lot @Blacksmoke16 🙏 CI is all green ✅ @Sija

@Blacksmoke16
Copy link
Contributor

Probably dont need to run ameba and format checks on both mac and ubuntu. I'd prob trim the jobs down to:

  • ameba - ubuntu-latest only crystal-latest
  • format - ubuntu-latest for both crystal-latest and crystal-nightly
  • specs - all 3 OS and latest and nightly crystal

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
Comment on lines 25 to 29
{% if flag?(:windows) %}
client_response.body.strip.should eq("<html>Hello world\r\n</html>")
{% else %}
client_response.body.strip.should eq("<html>Hello world\n</html>")
{% end %}
Copy link
Contributor

Choose a reason for hiding this comment

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

I really think there should not be any difference in the output format depending on whether the spec runs on Windows or Linux.
This discrepancy is most likely caused by hello.ecr being checked out with CRLF line ending on Windows, when it should use LF. Configuring gitattributes should fix that (probably needs rules for *.cr and *.ecr).

@sdogruyol sdogruyol force-pushed the windows-support branch 3 times, most recently from bc3148a to 32a31f8 Compare September 30, 2024 16:53
@sdogruyol
Copy link
Member Author

All green ✅ Thanks a lot @straight-shoota @Blacksmoke16 👍

@sdogruyol sdogruyol merged commit 85fcbbe into master Oct 1, 2024
18 checks passed
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.

5 participants