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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Drop py as a dependency #647

Merged
merged 9 commits into from Oct 7, 2022
Merged

Conversation

FollowTheProcess
Copy link
Collaborator

closes #583

I picked up @henryiii's excellent work on #592, resolved the conflicts and the issue he described with shutil.which expecting a .exe.

This should hopefully be everything needed to drop py.path, @henryiii I'd be grateful if you could check this out to see if I've solved the issue adequately as you have more of the context than me 馃檪

@FollowTheProcess FollowTheProcess added enhancement dependencies Issues relating to a nox dependency labels Aug 18, 2022
@FollowTheProcess FollowTheProcess self-assigned this Aug 18, 2022
@FollowTheProcess FollowTheProcess changed the title Drop py as a dependency refactor: Drop py as a dependency Aug 18, 2022
@henryiii
Copy link
Collaborator

Ah, so you just added the .exe in the tests? Is that fine to do? I could have done that too, but wanted to make sure it was actually correct behavior on Windows. I wasn't sure if removing the executable on Windows was valid for things in the local path. It is a small behavior difference.

Happy to have you finish this up (also always happy to give access to my fork if you need it)! I did four back-to-back conferences and am now preparing for scikit-build work and teaching in the Fall.

@FollowTheProcess
Copy link
Collaborator Author

Ah, so you just added the .exe in the tests? Is that fine to do?

My thinking was that this was really just a testing problem? shutil.which just like py.path.local.sysfind should be trusted to do the correct thing on either platform, it's just during a test we were constructing an executable without a .exe which shutil.which did not find on Windows (I think correctly) but py did.

AFAIK this shouldn't impact actual real world behaviour as on Windows, all executables have one of the PATHEXT (.exe, .bat etc.) extensions (not 100% this is a guarantee but in my experience it seems to hold true) and I think in this case, shutil.which will always find them?

@FollowTheProcess
Copy link
Collaborator Author

Anyone have any other comments/suggestions here? If not I'd like to get this in and drop an unneeded dependency 馃檪

Copy link
Collaborator

@DiddiLeija DiddiLeija left a comment

Choose a reason for hiding this comment

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

From my point of view, the fewer dependencies we have the better, so LGTM from me.
I also would like to hear other maintainers before we merge, however.

Copy link
Collaborator

@henryiii henryiii left a comment

Choose a reason for hiding this comment

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

I'd like to have verified this matches the standard behavior on Windows, but since it's how shutil works, I'm guessing it does. I'm very rarely on Windows so not worth waiting on me, I think.

@henryiii henryiii mentioned this pull request Oct 7, 2022
@FollowTheProcess
Copy link
Collaborator Author

I'd like to have verified this matches the standard behavior on Windows, but since it's how shutil works, I'm guessing it does.

Agreed, I think it's a sensible assumption. I also don't have a windows machine sadly

@DiddiLeija
Copy link
Collaborator

DiddiLeija commented Oct 7, 2022

I also don't have a windows machine sadly

I do! 馃槂

How can I help?

@FollowTheProcess
Copy link
Collaborator Author

How can I help?

Great! 馃帀

I think:

  • Make a fresh directory and make a simple script (could be just a simple bash script that echo's or something)
  • Either use windows CLI tools (powershell etc.) or python to turn this file into an executable (like what we do with chmod in the tests)
  • Ensure it doesn't have a .exe extension
  • Find out whether shutil.which("name_of_your_script_file") finds it or returns None (it should return None I think)
  • Put the .exe extension on and repeat above, shutil.which should now find it

Does that sound about right @henryiii?

@DiddiLeija
Copy link
Collaborator

Ok, I'm quite unfamiliar to Windows scripts and executables, but I'll give it a try. I'll post the results as soon as possible :)

@DiddiLeija
Copy link
Collaborator

DiddiLeija commented Oct 7, 2022

Hmm... I'm having issues to convert a script into an exe. I wrote both a .bat file and an extensionless file (but I suppose we wanted the extensionless option). If I use Python and write the current Nox' strategy:

import pathlib

exe = pathlib.Path("testexec")  # where "testexc" is an extensionless file that only contains "echo 'Hello world!'"
exe.chmod(0o700)
print(exe.stat().st_mode)

Another mode is returned (seems like Windows ignores any mode change excluding write and read modes, see https://docs.python.org/3/library/os.html?highlight=chmod#os.chmod). Any other way to convert a file to an exe using Windows?

@henryiii
Copy link
Collaborator

henryiii commented Oct 7, 2022

I'm quite sure shutil.which won't find it (that's why the test had to be changed), what I'd like to know if Windows itself picks it up. Try to run it manually with the current directory added to your path with and without the extension. If nox had a behavior not supported by the shell, I think it's perfectly fine to remove it.

@henryiii
Copy link
Collaborator

henryiii commented Oct 7, 2022

It could be the way Windows uses to detect "executable" is to always lookup PATHEXT, but I think it might try to run a file if you just enter it (I don't think it really has the concept of "executable mode" from UNIX).

Copy link
Collaborator

@henryiii henryiii left a comment

Choose a reason for hiding this comment

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

Yeah, I think @DiddiLeija's os.chmod link provided the answer - I don't think you can run an "extensionless" file in Windows (PATHEXT is required), and therefore the old behavior didn't match the way Windows worked. A little searching along those lines seems similar. So I think this change is safe - the stdlib is matching the OS behavior better than py.path did. Thanks!

@FollowTheProcess
Copy link
Collaborator Author

Thanks both! 馃帀

@FollowTheProcess FollowTheProcess merged commit cdd0f3b into main Oct 7, 2022
@FollowTheProcess FollowTheProcess deleted the refactor/drop-py branch October 7, 2022 18:39
@henryiii
Copy link
Collaborator

henryiii commented Oct 7, 2022

Not sure if anyone here is maintaining https://github.com/conda-forge/nox-feedstock/blob/a18e97ca0cc5221e403d54e43d246e89b641095f/recipe/meta.yaml#L27, but it will need to be updated on the next nox release. The URLs to the git repo is out of date too - CC @kynan, @MarckK, @tswast

@kynan
Copy link

kynan commented Oct 8, 2022

Thanks @henryiii. Note that this will only become relevant once a new nox release including this change hits PyPI (at which point automation should kick in and send me a PR).

@sscherfke
Copy link

When will this change be released? Currently, Safety complains because of https://pyup.io/v/51457/f17, so my pipelines are failing.

This is just for information. I'll find a workaround if you don't plan to do a release in the near future. :)

@DiddiLeija
Copy link
Collaborator

When will this change be released?

Right now 馃檭 (thank you @theacodes!)
PyPI builds should be available soon 馃槈

@henryiii
Copy link
Collaborator

Nice, this was needed for Python 3.11 in the action, too. ;)

@kynan
Copy link

kynan commented Nov 27, 2022

@henryiii new release on conda-forge also available now!

jcollado added a commit to jcollado/nox-feedstock that referenced this pull request Nov 30, 2022
py is no longer neeed by nox code according to:
https://github.com/wntrblm/nox/blob/main/CHANGELOG.md
wntrblm/nox#647

Not installing py in the first place is a good way avoid this
vulnerability:
pytest-dev/py#287
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Issues relating to a nox dependency enhancement
Development

Successfully merging this pull request may close these issues.

Remove usage of py.path?
5 participants