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

fix: Storage plugin bug on Windows #2487

Closed
wants to merge 12 commits into from
Closed

Conversation

melund
Copy link
Contributor

@melund melund commented Oct 20, 2023

Description

This PR attempts to fix the storage bug plugin test failures on Windows.
Fixes: #2473

@johanneskoester, I am not sure this is how you want to fix the bug, but it should give you an idea what the problem is.

Currently, on Windows all files in rules etc. are converted to forward slashes to avoid the 'backslash' mesh. However, the new storage plugin uses 'pathlib' for some paths internally. Converting pathlib.Path objects to strings gives paths with 'backslash'es on Windows. So I have added code to handle those cases.

I have also improved the CI test setup on windows. The setup time for conda environment creation is down from 12 min to 2.5 min on Windows. Much more is possible, but I have split that work into a separate PR (#2489)

The PR also does a few other things for local testing on Windows:

Turns ou that "Git for Windows" have reorganized its internal Unix tools so they are no longer on path (Things like "cp", "touch", "cat" etc.) I guess it works in the cloud because the GitHub actions VM comes prepackaged with all tools on path from MSYS package.

A few tests still fail for me locally but work in the cloud, but I haven't dug into that.

QC

  • The PR contains a test case for the changes or the changes are already covered by an existing test case.
  • The documentation (docs/) is updated to reflect the changes or this is not necessary (e.g. if the change does neither modify the language nor the behavior or functionalities of Snakemake).

This commit also makes vscode test plugin work, as test
discovery doesn't find all the "test_*" files which are not
supposed to be run by pytest directly
@@ -1,3 +1,5 @@
* text=auto eol=lf
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This ensures files are checked-out with linux file endings on Windows.That is usually not the recommendation for windows. But this is the best option given how many input-output files in the tests system that are manipulated by posix tools.

@@ -0,0 +1,2 @@
[pytest]
python_files = tests/tests.py tests/test_expand.py tests/test_io.py tests/test_schema.py tests/test_linting.py tests/test_executor_test_suite.py
Copy link
Contributor Author

@melund melund Oct 21, 2023

Choose a reason for hiding this comment

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

The test suite contains plenty of files which doesn't follow the standard pytest conventions. This tripped the vs-code test plugin which tried to inspect non-test files.

@melund melund changed the title Attempt to fix storage plugin bug on Windows fix: Attempt to fix storage plugin bug on Windows Oct 21, 2023
@melund melund force-pushed the master branch 2 times, most recently from 404f7ed to 4f72db4 Compare October 21, 2023 08:51
@melund melund force-pushed the master branch 2 times, most recently from e4810eb to 6c999ba Compare October 21, 2023 09:23
@melund melund changed the title fix: Attempt to fix storage plugin bug on Windows fix: Storage plugin bug on Windows Oct 21, 2023
@melund
Copy link
Contributor Author

melund commented Oct 21, 2023

@johanneskoester. Test pass and it is ready for review.

Copy link
Contributor

@johanneskoester johanneskoester left a comment

Choose a reason for hiding this comment

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

Thanks a lot! Sorry, the review was done since last week, but I forgot to hit the submit button. One thing below.

snakemake/io.py Show resolved Hide resolved
@johanneskoester
Copy link
Contributor

Seems like touching directories on windows does not work anymore with the changes here.

Copy link

sonarcloud bot commented Nov 14, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@melund
Copy link
Contributor Author

melund commented Nov 14, 2023

Seems like touching directories on windows does not work anymore with the changes here.

I will try to roll back so the check doesn't happen inside IOFile. The test were able to pass before I did that.

Comment on lines +736 to +739
local_path_str = str(storage_object.local_path())
if ON_WINDOWS:
local_path_str = local_path_str.replace("\\", "/")
file_with_wildcards_applied = IOFile(local_path_str, rule=self.rule)
Copy link
Contributor

Choose a reason for hiding this comment

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

The Path object has a method as_posix() which does exactly what you do here. This also applies to other cases below. Maybe we should just always use this method?

@johanneskoester
Copy link
Contributor

johanneskoester commented Nov 21, 2023

I have now implemented a simpler version of this in #2519. Thanks a lot for this work here, which pointed me to the right places for a simplified fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants