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.

Sign up for GitHub

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

Jump to bottom

Enable more win32 specs #8683

Merged
merged 13 commits into from
Feb 19, 2020
Merged

Enable more win32 specs #8683

merged 13 commits into from
Feb 19, 2020

Conversation

straight-shoota
Copy link
Member

Followup on #8670. This PR enables a lot of more specs for building on win32.

Often just a few specs needed to be disabled in order to make the rest of a spec file compile and pass. This affects mostly specs working with big numbers or yaml.

All specs pass on win32.

With this and #8668 we should get 6464 examples passing on win32, that's 70% of all standard lib specs.

@straight-shoota straight-shoota added platform:windows Windows support based on the MSVC toolchain / Win32 API kind:specs labels Jan 12, 2020
@straight-shoota
Copy link
Member Author 

Finished in 8.64 seconds
6466 examples, 0 failures, 0 errors, 57 pending

@straight-shoota straight-shoota marked this pull request as ready for review January 13, 2020 12:47
@straight-shoota straight-shoota force-pushed the fix/win32-specs-cont branch 2 times, most recently from 80b2b99 to 859e12a Compare January 13, 2020 14:42
Sija
Sija reviewed Jan 13, 2020
View reviewed changes
src/http/common.cr Outdated Show resolved Hide resolved
RX14
RX14 requested changes Jan 13, 2020
View reviewed changes
src/http.cr Show resolved Hide resolved
src/http/common.cr Outdated
raise "Can't decompress because `-D without_zlib` was passed at compile time"
case encoding
when "gzip"
when "deflate"
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure you don't mean when "gzip", "deflate"?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking again, if this wasn't caught by the specs, it should have been. Can we have a spec?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, this is not trivial because this branch is based on a compiler flag. The test would need to compile a separate program with -Dwithout_zlib for performing the HTTP parse. We don't have any specs for these flags at all yet. Given that they mostly comment out code and eventual replacements are usually very trivial, I'm not sure we should go that extra mile. Anyways, it should be discussed separately from this PR.

spec/std/io/io_spec.cr Outdated Show resolved Hide resolved
spec/std/io/memory_spec.cr Outdated Show resolved Hide resolved
spec/std/json/any_spec.cr Show resolved Hide resolved
src/logger.cr Outdated Show resolved Hide resolved
src/logger.cr Outdated Show resolved Hide resolved
spec/std/random_spec.cr Outdated Show resolved Hide resolved
spec/std/string_spec.cr Outdated
"こんいちは".reverse.should eq("はちいんこ")
end

it "reverses taking grapheme clusters into account" do
pending_win32 "reverses taking grapheme clusters into account" do
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this failing due to pcre not being compiled with unicode support on your system? I don't see why this spec should fail.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but I'm pretty sure I've built it with UTF-8 support as described in the porting guide.

Copy link
Contributor

Choose a reason for hiding this comment

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

How does it fail?

Copy link
Member Author

Choose a reason for hiding this comment

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

support for \P, \p, and \X has not been compiled in Argument 1

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that -DPCRE_SUPPORT_UNICODE_PROPERTIES=On needs to be used when compiling pcre for windows.

Copy link
Member

Choose a reason for hiding this comment

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

The flag is already there

crystal/.github/workflows/win.yml

Line 77 in 37d6047

cmake . -G "Visual Studio 16 2019" -DBUILD_SHARED_LIBS=OFF -DPCRE_SUPPORT_UNICODE_PROPERTIES=ON -DPCRE_SUPPORT_JIT=ON -DPCRE_STATIC_RUNTIME=ON
and these specs pass on CI, so they can just stay enabled.
oprypin@6a40de1

Copy link
Member

Choose a reason for hiding this comment

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

Updated the wiki to match

Copy link
Member Author

Choose a reason for hiding this comment

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

Awesome, so I just need to rebuild the library locally.

@RX14
Copy link
Contributor

RX14 commented Jan 13, 2020

force pushes :(

RX14
RX14 reviewed Jan 13, 2020
View reviewed changes
src/http/common.cr
require "./headers"
require "./content"
require "./cookie"
require "./formdata"
Copy link
Contributor

Choose a reason for hiding this comment

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

How about require "./**" to make sure everything's required?

Copy link
Member Author

Choose a reason for hiding this comment

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

That would break on windows.

Copy link
Contributor

@RX14 RX14 Jan 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

why?? I'd much much prefer you kept the requires the same and used skip_file on anything that breaks.

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'd like to avoid wildcard requires because it can lead to weird issues based on include-order. Writing a few explicit requires isn't bad IMO.

@straight-shoota
Copy link
Member Author

Sorry for the force pushes, but the changes were really insubstantial and this makes it easier to review for everyone who didn't take a look at it before.

@j8r j8r mentioned this pull request Jan 24, 2020
Merged
@straight-shoota straight-shoota requested a review from RX14 January 28, 2020 20:53
@straight-shoota
Copy link
Member Author

Rebased on master with #8676, now the specs run on GitHub Actions:

Finished in 1.92 seconds
6469 examples, 0 failures, 0 errors, 53 pending

@RX14
Copy link
Contributor

RX14 commented Feb 5, 2020

Just dropped a few notes in the conversations above.

RX14
RX14 approved these changes Feb 7, 2020
View reviewed changes
Copy link
Contributor

@RX14 RX14 left a comment

Choose a reason for hiding this comment

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

❤️

@RX14
Copy link
Contributor

RX14 commented Feb 7, 2020

rebase time!

@straight-shoota straight-shoota added this to the 0.34.0 milestone Feb 18, 2020
@straight-shoota
Copy link
Member Author

Rebased and squashed.

@straight-shoota
Copy link
Member Author

Somehow win32 specs hang at some point. I've now added verbose option. Next step is to disable IO specs because that's where execution seems to stop.

@straight-shoota straight-shoota force-pushed the fix/win32-specs-cont branch 2 times, most recently from ed677af to bf9e5b0 Compare February 18, 2020 20:12
@straight-shoota straight-shoota merged commit ae9257a into crystal-lang:master Feb 19, 2020
@straight-shoota straight-shoota deleted the fix/win32-specs-cont branch February 19, 2020 10:19
@straight-shoota straight-shoota mentioned this pull request Feb 19, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oprypin oprypin oprypin left review comments

@Sija Sija Sija left review comments

@RX14 RX14 RX14 approved these changes

@bcardiff bcardiff bcardiff approved these changes

Assignees
No one assigned
Labels
kind:specs platform:windows Windows support based on the MSVC toolchain / Win32 API
Projects
None yet
Milestone
0.34.0
Development

Successfully merging this pull request may close these issues.
5 participants
@straight-shoota @RX14 @Sija @oprypin @bcardiff