Skip to content

Win CI: Fix and enable compiler specs#9348

Merged
jhass merged 6 commits intocrystal-lang:masterfrom
oprypin:wincospec
Jun 23, 2020
Merged

Win CI: Fix and enable compiler specs#9348
jhass merged 6 commits intocrystal-lang:masterfrom
oprypin:wincospec

Conversation

@oprypin
Copy link
Member

@oprypin oprypin commented May 23, 2020

No description provided.

@oprypin

This comment has been minimized.

@oprypin oprypin force-pushed the wincospec branch 3 times, most recently from c436c9d to f142b31 Compare May 25, 2020 18:49
@oprypin
Copy link
Member Author

oprypin commented Jun 2, 2020

Finally, now that there are no dependencies on other PRs, I squashed all that merge-mess :>

@RX14
Copy link
Member

RX14 commented Jun 5, 2020

I'm a bit confused as to the reason for many of these failing specs.

I'd rather not make specs pending if we don't understand why they're broken, so could you give some details?

@oprypin
Copy link
Member Author

oprypin commented Jun 5, 2020

Currently all of the specs are effectively pending, I just reduce that to a dozen.

I really just don't know why these things don't work. There are no stack traces. I'm tired.
So no, I don't want to look for an explanation.

Where I did know, I already fixed them. Many of them.

Now I just want to stop the bleeding addition of more failing specs.

Additionally, I consider merging fixes to these specs before enabling specs as strictly worse; then the fix PRs have no representation in specs even if something happens to be fixed (while with this in you just flip it back from pending). Showing the specs as what they are is good.

Best (bad) example: #9387 changes specs for seemingly no reason, but actually they were failing before, if they were to run. Then it even marks a related spec as pending, as if the commit breaks that spec???

Copy link
Member

@jhass jhass left a comment

Choose a reason for hiding this comment

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

This looks good! Windows support is a big task and any incremental improvements are good.

@oprypin
Copy link
Member Author

oprypin commented Jun 5, 2020

For the record, this PR is inconsequential for the 0.35.0 release (changes only specs), so definitely feel free to postpone it to after that

@jhass jhass added pr:ready-to-merge The changes are good to go, we need to triage merging it. kind:feature platform:windows Windows support based on the MSVC toolchain / Win32 API labels Jun 5, 2020
@straight-shoota
Copy link
Member

I'd rather not make specs pending if we don't understand why they're broken

They've essentially been pending all the time because they're not included in win32_std_spec.cr. So this doesn't change anything for those specs that are now pending_win32. But it does change the specs that are no longer pending, because they work.

Thanks for your efforts @oprypin

Copy link
Member

@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.

Yeah, you're right, this is better, my bad!

@jhass jhass merged commit 956f401 into crystal-lang:master Jun 23, 2020
@jhass
Copy link
Member

jhass commented Jun 23, 2020

🎉

@jhass jhass added this to the 1.0.0 milestone Jun 23, 2020
@jhass jhass removed the pr:ready-to-merge The changes are good to go, we need to triage merging it. label Jun 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind:feature platform:windows Windows support based on the MSVC toolchain / Win32 API topic:infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants