-
Notifications
You must be signed in to change notification settings - Fork 27
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
feat: Add Windows support #591
Conversation
There appears to be a few problems in the test suite, but the core functionality now works for Windows. Interestingly, the errors seen on GitHub do not occur on my own Windows machine. @algomaster99 could you try running the tests of this branch on your Windows machine and see what happens? |
I will check it by tonight. |
No hurry, do it when you've got time. |
@slarse I was able to repair the files which @PrakashNayak provided. However, the tests are failing on my system too. [ERROR] Tests run: 434, Failures: 5, Errors: 7, Skipped: 2
Failures have long messages so let me know if you need me to send it you. |
Those are similar errors to CI but not the same:
Some of the errors you get are most likely due to not having Maven on the system path (I get those on my machine). The creation of the statistics file also appears not to work, and the violation specifier appears a bit broken. |
To as quickly as possible add at least partial support for Windows, I'm just ignoring the test failures. The vast majority of functionality seems to be working fine. |
On Windows, there is a difference between the system path and the user path. When using Python's |
Ping @fermadeiral @khaes-kth, ready for review/merge |
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.
LGTM.
@@ -52,6 +56,7 @@ jobs: | |||
restore-keys: ${{ runner.os }}-sonar | |||
|
|||
- name: Check formatting with spotless | |||
if: ${{ matrix.os == 'ubuntu-latest' }} |
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.
Instead of this if condition, we can probably set core.eol
to lf
in a .gitattributes
file so that spotless doesn't change the line endings while formatting. This should be done when we decide that sorald should support development on Windows too.
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.
That's not a full solution, as if we fully support Windows, we also need to have files with CRLF line endings in our test resources. To test the sniper printer output, that is. We literally need Git not to change line endings or the tests break.
That being said, we could of course just set core.autocrlf false
in a checked-in .gitattributes
file.
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.
we also need to have files with CRLF line endings in our test resources. To test the sniper printer output, that is.
Right. I did not consider this. 😅
fix #589, fix #335
Partially addresses #273
This PR adds core support for Windows, along with a Windows build in the CI. Note that statistics gathering, targeted repair and segment mode tests are not passing: they are currently disabled on Windows.