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: "python ABS/PATH/TO/ipython.py" fails #3276

Merged
merged 1 commit into from May 10, 2013
Merged

Conversation

tkf
Copy link
Contributor

@tkf tkf commented May 4, 2013

The following line in setupbase.py was the problem. It assumes
that your CWD is always at the repository root. This patch removes
this assumption.

execfile(pjoin('IPython','core','release.py'), globals())

@bfroehle
Copy link
Contributor

bfroehle commented May 4, 2013

Is the abspath necessary?

Also, there is a small precedent in the Python community to call this __directory__, i.e,:

__directory__ = os.path.dirname(__file__) 

@tkf
Copy link
Contributor Author

tkf commented May 4, 2013

No, abspath is not necessary but I tend to do this because it works even if you live with evil functions which changes directory. Also I don't have strong opinion about the naming so it is OK to rename it to __directory__ (but I don't know if it is a good idea to define variable with double underscores which is not in some kind of spec).

BTW, what is the policy for merging PRs? Does it need more than two reviewers or one reviewer is fine? I just quickly checked the wiki but there is not much info. If one reviewer is fine I would just fix them right away, because this kind of stuff is mostly about preference and I think comitters preference must be weighted than mine. But if more than two reviewers are needed, waiting for another comment is better because other reviewer might have different opinion. I am just being lazy :)

@function2
Copy link

I changed it to
-execfile(pjoin('IPython','core','release.py'), globals())
+execfile(pjoin(os.path.dirname(sys.argv[0]),'IPython','core','release.py'), globals())

If I may say so I really don't like exec* at all...

@minrk
Copy link
Member

minrk commented May 5, 2013

Also for reference, I think we are going to remove ipython.py before 1.0.

@ellisonbg
Copy link
Member

Also, please use standard variable_names rather than all caps. With ipython.py being removed, what do we want to do with this one?

@tkf
Copy link
Contributor Author

tkf commented May 10, 2013

There are decisions to make before I can make a change. Please give me your choices. I wrote my opinion in the above comment.

  1. repo_root or __directory__
  2. Should we use abspath?

@fperez
Copy link
Member

fperez commented May 10, 2013

  1. Use repo_root: no caps, and there's always a chance Python will in the future define __directory__ for something, so let's stay away from that.
  2. Do use abspath: it's a good safety to use in this kind of code.
  3. There's a conflict right now that needs fixing, should be an easy rebase.

Thanks!

The following line in setupbase.py was the problem.  It assumes
that your CWD is always at the repository root.  This patch removes
this assumption.

    execfile(pjoin('IPython','core','release.py'), globals())
@tkf
Copy link
Contributor Author

tkf commented May 10, 2013

Rebased. Thanks for your review!

@fperez
Copy link
Member

fperez commented May 10, 2013

OK, looks good now. Thanks! Merging now.

fperez added a commit that referenced this pull request May 10, 2013
Remove assumption that setup is being run from repo root.
@fperez fperez merged commit 027d51b into ipython:master May 10, 2013
@tkf tkf deleted the fix-setupbase branch May 10, 2013 22:43
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
Remove assumption that setup is being run from repo root.
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.

None yet

6 participants