Skip to content

Make the tests work under Windows #943

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
Closed
rvogg wants to merge 3 commits into from
Closed

Make the tests work under Windows #943

rvogg wants to merge 3 commits into from

Conversation

@rvogg
Copy link
Contributor

@rvogg rvogg commented Oct 6, 2021 •
edited
Loading

With these changes I made some adaptations that the tests run under Windows again.
Tests that fail and cannot be fixed quickly have been deactivated for the moment.
Additionally I added mingw-w64 builds under Windows, because the msvc builds contain other bugs.

In some places I had to give the shell scripts a '.sh' extension to make them run under Windows. It looks like this does not affect the other operating systems.
( CCACHE_DETECT_SHEBANG does not work correctly -> The tests produce a "could_not_find_compiler" error if the .sh extension is missing.)

I hope this change helps avoid breaking existing behavior on Windows and make ccache even better on Windows.

Note: The secondary_http test fails sporadically on Windows. (Perhaps it should also be disabled)

@louiscaron
Copy link
Contributor

louiscaron commented Oct 19, 2021 •
edited
Loading

This looks good to me. Actually, i had to implement some very small portions of it in order to implement and run on Windows the testcases for the new compiler I would like to add support for (#942).

If, by chance, your patch gets merged soon I would rebase my own patch. thanks

@rvogg rvogg force-pushed the fix-windows-tests branch 2 times, most recently from 5e61de3 to 473f583 Compare October 26, 2021 20:01
llunak
llunak reviewed Nov 1, 2021
View reviewed changes
@rvogg rvogg mentioned this pull request Nov 2, 2021
Merged
6 tasks
@jrosdahl
Copy link
Member

jrosdahl commented Nov 3, 2021

With these changes I made some adaptations that the tests run under Windows again.

Note that the Windows tests are work in progress and have never worked before, so it's not the case that they have become broken.

@jrosdahl jrosdahl added improvement Improvement that is not a bug fix or new feature test Affects testing of Ccache itself labels Nov 3, 2021
@rvogg
Copy link
Contributor Author

rvogg commented Nov 3, 2021

With these changes I made some adaptations that the tests run under Windows again.

Note that the Windows tests are work in progress and have never worked before, so it's not the case that they have become broken.

I thought that because there were many if ! $HOST_OS_WINDOWS are spread over the tests, it was less broken than now.
Currently not a single test runs on Windows because they all fail on the expect_stat function.

My initial motivation for this change was: I started to fix the basedir functionality for Windows, because it doesn't really work with all the Windows path mess.
After a few #ifdef __Win32 changes I realized that I broke a lot more but couldn't really check what. So I first tried to get to a state where I could verify my changes.

I didn't fix all tests, because in some cases ccache really doesn't work on Windows or the effort to get it running in the git-bash was too high. And I don't expect to fix all bugs for ccache under Windows, but only that no additional bugs get in by mistakes.

@rvogg rvogg force-pushed the fix-windows-tests branch from 473f583 to 9239fce Compare November 9, 2021 14:57
@rvogg
Copy link
Contributor Author

rvogg commented Nov 9, 2021

I added the commit 'handle newlines on Windows properly' from #954. With this I was able to remove some hacks.

I did not add the commit 'handle properly also stdout in addition to stderr' because it breaks the correct ccache behavior.
With export CCACHE_NOCPP2=1 the preprocessor output is written to stdout.

@rvogg rvogg force-pushed the fix-windows-tests branch from 9239fce to 1125d8c Compare November 17, 2021 14:27
rvogg added 3 commits November 29, 2021 16:17
@rvogg
test(Windows): Adapt the test scripts to be able to run on Windows.
a9017c1 
- Adding the ".sh" extension to most shell scripts.
  It looks like Windows needs this to run the scripts.

- Adding special handling of carriage return characters.

- Tests that fail are deactivated for the moment.
@rvogg
ci(Windows): Add additional runners in different Msys2 environments.
932d3da
@rvogg
test: Disable the sporadic secondary_http tests 'Basic auth required'…
03d2079 
… and 'Basic auth failed'.
@rvogg rvogg force-pushed the fix-windows-tests branch from 9808365 to 03d2079 Compare November 29, 2021 15:21
@jrosdahl jrosdahl changed the title Made the tests work again under Windows. Made the tests work again under Windows Dec 14, 2021
@jrosdahl jrosdahl changed the title Made the tests work again under Windows Make the tests work under Windows Dec 14, 2021
@jrosdahl
Copy link
Member

jrosdahl commented Dec 14, 2021

@rvogg wrote:

I thought that because there were many if ! $HOST_OS_WINDOWS are spread over the tests

I understand. @nickhutchinson made several Windows-related improvements (#757, #780, #789, maybe more) of the test suite, but it was never enabled in CI so I don't think it worked fully.

My initial motivation for this change was: I started to fix the basedir functionality for Windows, because it doesn't really work with all the Windows path mess.

Regarding that, I think that the way to make basedir work in a reasonable cross-platform way is to start using std::filesystem::path internally instead of handling paths and normalization spread throughout the code. See also #866.

@orgads
Copy link
Contributor

orgads commented Aug 14, 2022

@rvogg ping

@orgads orgads mentioned this pull request Aug 14, 2022
Merged
@orgads
Copy link
Contributor

orgads commented Aug 14, 2022

Rebased and created a new PR #1133

@rvogg rvogg closed this Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@llunak llunak llunak left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

improvement Improvement that is not a bug fix or new feature test Affects testing of Ccache itself

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@rvogg @louiscaron @jrosdahl @orgads @llunak