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 breaking pathlib change in 3.10 beta1 #594

Merged
merged 9 commits into from
May 14, 2021

Conversation

carlmontanari
Copy link
Contributor

Hello!

Sorry for the PR outa the blue w/out an issue to discuss -- had some time and figured I'd take a crack at it. I added 3.10 beta1 to my ci for some projects using pyfakefs and my builds are failing. Did a bit of sleuthing and it seems there were a few changes to Pathlib for this release. The main issue/thread seems to be here -- and associated commit. This commit also appears to be related.

I think (hope?!) that this fix is backwards compatible. Tests in the docker container all seem happy, and I've tested this locally on my Mac w/ 3.10 beta1 and the tests that are/were failing in CI pass like normal. For reference, here is my failed tests (the build_posix (ubuntu-latest, 3.10.0-beta.1) job).

The fix itself is basically to ignore that init argument (the pr/issue on Cpython seems to say this is mostly legacy and (obviously) going away anyway). Then allowing the wrapped stat method (well I guess everything... which is perhaps not ideal?) to accept kwargs as well. Since _init method is no longer called, I removed that and just assigned that _accessor to the _fake_accessor directly after the _from_parts calls.

Hope that all makes sense -- have been happily using pyfakefs (thanks for the project btw!) a while but the guts of it are probably a bit over my head :)

Carl

Copy link
Member

@mrbean-bremen mrbean-bremen left a comment

Choose a reason for hiding this comment

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

Thank you, this looks good to me apart from that minor change! Can you also please add an entry in the release notes?
If you want, you could also add 3.10-dev to the test matrix in the workflow.

@mrbean-bremen
Copy link
Member

mrbean-bremen commented May 10, 2021

I had actually planned to test Python 3.10-dev with pyfakefs after I have seen some changes that break for example pytest, so this is a nice contribution.
I just noticed that the GH actions are not running after the PR - looks like I haven't enabled them - I will fix this...
EDIT: Ok, done. You may want to rebase with master if you want to change the workflow file, too.

@carlmontanari
Copy link
Contributor Author

If you want, you could also add 3.10-dev to the test matrix in the workflow. <- had to add 3.10.0-beta1 explicitly, but this has been added. I tried to just put 3.10-dev but it wasn't having it -- maybe because pre release? (tried over in my scrapli_cfg repo if you wanted to check out the build log).

Changelog updated, and fixed the _closed bits. Let me know if there's anything else that needs doin!

edit looks like we may need to update the python actions to v2 as well. I will update that here in just a sec!

@carlmontanari
Copy link
Contributor Author

Just pushed a commit that restores the _init method.... for whatever reason it seems that its working. I would have assumed because it was being called in __new__ we would have been hitting these lines where we set the accessor and closed anyway? Can circle back on this tomorrow with fresh eyes if that didn't work for some reason :D

# same comment as above for these two
realpath = staticmethod(os.path.realpath)
getcwd = staticmethod(os.getcwd)

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 and the above section I am 99% sure I am doing something wrong/bad here -- but the gist of this is it looks like we needed to add some methods to the FakeAccessor object -- I am assuming(?) its not a great idea to just attach os methods here because we would want to have the accessor using the patched methods? But this does seem to get more of the tests passing so figured id commit it to try to be as helpful as I can... (even if it isn't much :))

Copy link
Member

Choose a reason for hiding this comment

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

I changed it accordingly. Regarding getcwd - if using os inside the function, as I do it now, it will be patched, so it will behave correctly.

Copy link
Member

Choose a reason for hiding this comment

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

I removed realpath as it doesn't seem to be really needed, but maybe I'm wrong here.

@carlmontanari
Copy link
Contributor Author

Last push for the day because I am def out of my element here :) I left some comments in the files changed section about things im not super sure about and/or I think are probably wrong. This does get tox tests passing locally but some of it feels fairly wrong. Hopefully this can at least save ya some time on figuring stuff out!

@mrbean-bremen
Copy link
Member

mrbean-bremen commented May 11, 2021

@carlmontanari - unfortunately, I don't have much time today, but I had a quick look. What I can see is that there are other tests failing because of other changes in Python 3.10, so I don't expect you to fix it all. There are also a few tests failing in 3.9 now, but I haven't really looked into this yet. Hopefully will have some time tomorrow.

I can make commits into your PR to fix some of the problems if you don't mind (not today though), or help you to fix it,
or make a new PR for the changes - I leave that to you. One way or another, your PR was already helpful to go ahead with Python 3.10 support!

@carlmontanari
Copy link
Contributor Author

@mrbean-bremen Sorry I didn't get a chance to look today either, and sorry for my barrage of not working commits :). If you dont get to it first I will try to spend some more time looking at this this weekend but I think I am in over my head! Whatever is easiest for you w/ respect to the PR I'm totally good w/! I'm glad to have at least helped a little bit!

@mrbean-bremen
Copy link
Member

@carlmontanari -- thanks! There is no need for you to spend more time on this, I can take it from here. As I wrote, your contribution has already been helpful enough. I'll work on your branch then if you don't mind and will see if I can finish it soon -- maybe at the weekend.

@mrbean-bremen mrbean-bremen force-pushed the 3.10_pathlib_support branch 5 times, most recently from 1c20274 to 089da72 Compare May 13, 2021 16:55
@mrbean-bremen
Copy link
Member

@carlmontanari - I got it to work now, I'll let you have a look and check if something is missing from your POV before I'm going to merge it. Didn't have to wait until the weekend, as today was a holiday here...

@carlmontanari
Copy link
Contributor Author

carlmontanari commented May 13, 2021

@carlmontanari - I got it to work now, I'll let you have a look and check if something is missing from your POV before I'm going to merge it. Didn't have to wait until the weekend, as today was a holiday here...

You rock! A quick check and looks like all my tests are passing on 3.10 locally. Will try to get a branch updated to remove my skips to let it run on 3.10 on ubuntu latest too (locally am macOS).

edit all tests in ci on ubuntu-latest for one of my projects work as expected in 3.10, so think we are lookin good!

carlmontanari and others added 9 commits May 13, 2021 21:19
- fix link_to for Python 3.10
- revert some of the probably unneeded changes
- handle dummy encoding "locale" introduced in Python 3.10
- it calls chmod() in Python 3.10, so this has to handled
- some dependencies are not available for the beta version
- comment out docker tests - setup currently fails, has to be handled separately
@mrbean-bremen mrbean-bremen force-pushed the 3.10_pathlib_support branch from 049e2fd to 86bb5f7 Compare May 13, 2021 19:21
@mrbean-bremen mrbean-bremen merged commit a8a70bb into pytest-dev:master May 14, 2021
github-actions bot pushed a commit that referenced this pull request May 14, 2021
…I - adapt fake pathlib, fix pathlib.Path methods link_to, getcwd, lchmod - handle dummy encoding "locale" introduced in Python 3.10 - do not test extra dependencies with Python 3.10 (some are not available)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants