Skip to content

Add argument for cpython clone #69

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

Closed
warsaw opened this issue Apr 7, 2017 · 6 comments
Closed

Add argument for cpython clone #69

warsaw opened this issue Apr 7, 2017 · 6 comments

Comments

@warsaw
Copy link
Member

warsaw commented Apr 7, 2017

I already have a cpython clone elsewhere, so it would be nice if I didn't have to clone it again into my cherry_picker directory. Can I please have an option to point cp to an existing clone?

@Mariatta
Copy link
Member

Mariatta commented Apr 8, 2017

So there is a hidden functionality that allows you to run cherry_picker.py from an existing cpython directory. @warsaw Do you think you can work with this for now?

python <path to cherry_picker.py> <commit_hash> <branches>

I have just added this to readme in this pull request #70
Thanks.

@ncoghlan
Copy link
Contributor

ncoghlan commented Apr 9, 2017

While I merged #70, it turns out this approach technically isn't quite right now that the cherry_picker directory is a package, rather than just a directory holding the cherry_picker.py file.

A command like:

python ../core-workflow/cherry_picker/cherry_picker.py --push pr 2abfdf5 3.6 3.5

breaks the general principle of "don't run modules inside packages as if they were standalone scripts" (which became a guideline as ignoring it does odd things to sys.path and means you can import the same module twice under different names).

There are a few possible options for resolving this, and I've listed each of them below with the resulting permitted invocations:

  1. Remove the __init__.py file again and require anyone using the -m form to ensure that core-workflow/cherry_picker/ is on sys.path.
# If in core-workflow/cherry_picker/
python cherry_picker.py <args>
python -m cherry_picker <args>
# In a CPython checkout at the same level as the core-workflow checkout
python -m ../core-workflow/cherry_pickercherry_picker <args>
PYTHONPATH=../core-workflow/cherry_picker python -m cherry_picker <args>
  1. Keep the __init__.py, but rename cherry_picker.py to __main__.py, giving an executable package rather than an executable directory and hence requiring the use of the -m switch.
# Avoid running from core-workflow/cherry_picker/
# If in core-workflow/
python -m cherry_picker <args>
# In a CPython checkout at the same level as the core-workflow checkout
PYTHONPATH=../core-workflow/cherry_picker python -m cherry_picker <args>
  1. Make cherry_picker/ an executable directory instead of a package directory, by removing the __init__.py and either renaming cherry_picker.py to __main__.py, or else arranging for the latter name to run the former.
# If in core-workflow/cherry_picker/
python `pwd` <args>
# If in core-workflow/
python cherry_picker <args>
# In a CPython checkout at the same level as the core-workflow checkout
python ../core-workflow/cherry_picker <args>
# In a directory with both core-workflow and cpython as children
python core-workflow/cherry_picker <args>

My personal preference would be for the last one, as it's the only option where the relative path to the directory to run and the CPython checkout being either the current directory or a child directory called cpython are the only things that depend on the current directory (and even the latter could be made optionally absolute by offering a --workdir option that indicates the local repo to use).

@ncoghlan
Copy link
Contributor

ncoghlan commented Apr 9, 2017

I forgot to mention another potential benefit of the third option: it sets us up to bundle cherry-picker with all its dependencies as an executable zipapp archive.

@ncoghlan
Copy link
Contributor

#64 raises a 4th option: given a basic setup.py file, the cherry-picker could be installed with pipsi, and carry its own environment with it.

Regardless of which option we choose, the test.py script will likely require some adjustment.

@Mariatta
Copy link
Member

Once #99 is merged, this won't be necessary, since people can do the cherry-pick from cpython directory.

@Mariatta
Copy link
Member

With the update made in #99 you can now cherry-pick from within cpython directory, so a new argument is not necessary.

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

No branches or pull requests

3 participants