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

Fixes for Windows support #22

Merged
merged 6 commits into from
Aug 3, 2024
Merged

Fixes for Windows support #22

merged 6 commits into from
Aug 3, 2024

Conversation

frichtarik
Copy link
Contributor

@frichtarik frichtarik commented Apr 17, 2024 •
edited
Loading

  • this switches posix.DirEntry to os. DirEntry, which is os-independent proxy as suggested by https://docs.python.org/3/library/posix.html#module-posix

  • adding windows image to the GH workflow matrix

  • limiting codecov publisher step to one job from the matrix, no need to republish the same comment too many times

@codecov-commenter
Copy link

codecov-commenter commented Apr 17, 2024 •
edited by codecov bot
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (50b35fe) to head (f0a4386).
Report is 2 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff            @@
##            master       #22   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            7         7           
  Lines          350       348    -2     
  Branches        79        60   -19     
=========================================
- Hits           350       348    -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@frichtarik frichtarik marked this pull request as draft April 17, 2024 12:51
@frichtarik frichtarik force-pushed the windows_fix branch 13 times, most recently from 6a4c859 to 851febc Compare April 22, 2024 13:30
@frichtarik frichtarik marked this pull request as ready for review April 22, 2024 13:41
@frichtarik
Copy link
Contributor Author

@andhus #4 this is a draft on how to fix the windows compatibility (partially reverting/optimizing #11 )

i've set 5 tests to skip on Windows and adjusted some literals to be os agnostic
this causes tests to pass, circumventing some Windows peculiars, for the price of decreasing coverage (not sure why as that should be counted on Linux environment)

feel free to comment/adjust

@frichtarik
Copy link
Contributor Author

frichtarik commented Apr 22, 2024 •
edited
Loading

@andhus could you please create a new release of the dirhash soon? currently there is a conflicting requirement of pathspec in scantree vs dirhash

  • with the renaming of workflow, job names changed and now PR expects 6 non-existing checks to finish

@frichtarik frichtarik mentioned this pull request Apr 22, 2024
Closed
bossanova808 added a commit to bossanova808/repository.bossanova808 that referenced this pull request Apr 22, 2024
@bossanova808
Revert to dirhash, there is a Windows fix pending for it...
f8b1f3e 
...which might fix the cross-environment issues?
See: andhus/scantree#22
@andhus
Copy link
Owner

andhus commented Apr 23, 2024

@andhus could you please create a new release of the dirhash soon? currently there is a conflicting requirement of pathspec in scantree vs dirhash

Oh, yes (!) I missed this one: https://github.com/andhus/scantree/blame/c50ed7dc85f30b2a9bcad2965e867087555cf29d/setup.py#L27

@andhus
Copy link
Owner

andhus commented Apr 23, 2024

⬆️ https://github.com/andhus/dirhash-python/releases/tag/v0.4.0

andhus
andhus reviewed Apr 23, 2024
View reviewed changes
Copy link
Owner

@andhus andhus left a comment

Choose a reason for hiding this comment

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

Great stuff @frichtarik, thanks!

Please see a few small comments/questions.

tests/test_node.py Outdated Show resolved Hide resolved
tests/test_node.py Show resolved Hide resolved
tests/test_path.py Outdated Show resolved Hide resolved
tests/test_path.py Outdated Show resolved Hide resolved
tests/test_scantree.py
@@ -163,6 +165,7 @@ def rp(relative):
tree_empty_false_expected = DirNode(path=rp(""))
assert tree_empty_false == tree_empty_false_expected

@pytest.mark.skipif(os.name == "nt", reason="Windows runs too slow for set limits")
Copy link
Owner

Choose a reason for hiding this comment

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

Interesting, can take a look at this separately.

src/scantree/test_utils.py Show resolved Hide resolved
src/scantree/test_utils.py Outdated Show resolved Hide resolved
@andhus
Copy link
Owner

andhus commented Apr 23, 2024

with the renaming of workflow, job names changed and now PR expects 6 non-existing checks to finish

Yes, will update those after the merge of this PR 👍

@racinmat racinmat mentioned this pull request Apr 25, 2024
Closed
@frichtarik
Copy link
Contributor Author

sorry for the delay, I got to switch employer i am was working for and switch dev machine 🤦
after last fixes, there is one test that fails on LIN but not WIN, going to fix that later today and we should be good to go

@bossanova808
Copy link

Would the lack of this PR be why using this in two WSL environments seems not to work at all?

Dirhashes seem to be always different across the environments (each is pointing at a Windows drive, e.g.: /mnt/e/Git/Kodi/repository.bossanova808

@frichtarik
Copy link
Contributor Author

It took longer than expected, but i'm finally done

@frichtarik frichtarik requested a review from andhus June 22, 2024 11:08
@mparanjpe mparanjpe mentioned this pull request Jul 4, 2024
Closed
@racinmat
Copy link

racinmat commented Jul 9, 2024 •
edited
Loading

@andhus how does it look with the merging?

@andhus andhus changed the title windows fix Fixes for Windows support Aug 3, 2024
@andhus andhus merged commit cdad980 into andhus:master Aug 3, 2024
13 checks passed
@andhus
Copy link
Owner

andhus commented Aug 3, 2024

Sorry for the delay!

(New family member + ambitious startup + vacation... Additional maintainers are always welcome ;))

This was referenced Aug 3, 2024
Closed
Closed
Closed
@gaby
Copy link
Collaborator

gaby commented Aug 3, 2024

@andhus Doing a new release?

@andhus
Copy link
Owner

andhus commented Aug 3, 2024

with the renaming of workflow, job names changed and now PR expects 6 non-existing checks to finish

Yes, will update those after the merge of this PR 👍

Done ✅

@andhus andhus mentioned this pull request Aug 3, 2024
Merged
@andhus
Copy link
Owner

andhus commented Aug 3, 2024 •
edited
Loading

@frichtarik I added similar fixes to andhus/dirhash-python#29

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andhus andhus Awaiting requested review from andhus

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@frichtarik @codecov-commenter @andhus @bossanova808 @racinmat @gaby