Opened 8 years ago
Closed 6 years ago
#15100 closed enhancement (duplicate)
ipython unnecessarily relies on sage-location
Reported by: | felixs | Owned by: | |
---|---|---|---|
Priority: | minor | Milestone: | sage-duplicate/invalid/wontfix |
Component: | packages: standard | Keywords: | |
Cc: | jondo | Merged in: | |
Authors: | Reviewers: | Jeroen Demeyer | |
Report Upstream: | N/A | Work issues: | |
Branch: | u/felixs/ipython (Commits, GitHub, GitLab) | Commit: | 9c372862bc86c6aa09b9fbf94711405d716abff7 |
Dependencies: | #15146 | Stopgaps: |
Description (last modified by )
The interpreter line exchange for ipython should be done within spkg-install. The hack cannot be removed from sage-location otherwise.
Change History (24)
comment:1 Changed 8 years ago by
- Status changed from new to needs_review
comment:2 Changed 8 years ago by
- Milestone changed from sage-5.12 to sage-6.0
comment:3 Changed 8 years ago by
- Cc jondo added
comment:4 Changed 8 years ago by
- Status changed from needs_review to needs_work
comment:5 follow-up: ↓ 6 Changed 8 years ago by
I don't fully understand. In which way does sage-location
have nothing to do with ipython
if it rewrites the first line of ipython
?
What else needs to be done in your opinion?
comment:6 in reply to: ↑ 5 Changed 8 years ago by
Replying to felixs:
I don't fully understand. In which way does
sage-location
have nothing to do withipython
if it rewrites the first line ofipython
?
It has nothing to do with ipython
because $SAGE_LOCAL/bin/ipython
is only one of the many scripts which are rewritten by sage-location
.
Perhaps you should explain why the "hack" in sage-location
should be removed.
comment:7 follow-ups: ↓ 8 ↓ 9 Changed 8 years ago by
Every hack should be removed. Expecially the ones that implement things that can be achieved easily without hacks.
Hacks are often misleading and complicate issues. Getting rid of them needs more work, but this ticket is meant to just handle this particular case, I came across.
The whole story is: a doctest checks the first line in $SAGE_LOCAL/bin/ipython
. This doctest will certainly fail, if ipython has been disabled (toplevel build system, #14796). That's why #14796, depends on #15105 (unhardwire paths). In the ipython
case, unhardwiring paths can be simply achieved by removing the doctest. With this ticket, there is no doubt about how the first line in ipython
looks like (in case it came from sage) and the doctest is unnecessary.
comment:8 in reply to: ↑ 7 ; follow-up: ↓ 10 Changed 8 years ago by
- Description modified (diff)
Replying to felixs:
Every hack should be removed. Expecially the ones that implement things that can be achieved easily without hacks.
Fine, go ahead and implement sage-location
without hacks...
By special-casing ipython
you are achieving absolutely nothing: the "hack" in sage-location
still exists and you're just fooling the doctest.
comment:9 in reply to: ↑ 7 Changed 8 years ago by
Replying to felixs:
The whole story is: a doctest checks the first line in
$SAGE_LOCAL/bin/ipython
.
Sure, but even this doctest has nothing to do with ipython
. It is only meant to check that sage-location
worked.
comment:10 in reply to: ↑ 8 Changed 8 years ago by
Replying to jdemeyer:
Fine, go ahead and implement
sage-location
without hacks...
This is one part of it. Tickets must be achievable.
By special-casing
ipython
you are achieving absolutely nothing: the "hack" insage-location
still exists and you're just fooling the doctest.
I have fooled a *bogus* doctest. That's fine with me. What else do you suggest?
comment:11 Changed 8 years ago by
- Dependencies set to #15146
- Status changed from needs_work to needs_review
comment:12 Changed 8 years ago by
- Status changed from needs_review to needs_info
I still don't understand the point of this ticket. I think you should better explain what the problem is and why your patch fixes that problem.
comment:13 follow-up: ↓ 14 Changed 8 years ago by
sage-location
is meant to rewrite hardwired paths that contain $SAGE_OLD_ROOT
, which is a sort of workaround (or, in the unconditional case, just "hack"). ipython
does not depend on $SAGE_ROOT
and there is no point in requiring this mechanism. Moreover, if spkg-install
is self-contained, somebody might be able to read it and deduce how ipython
on sage is installed and intended to work.
(I agree that there are worse issues, but this one is particularly easy to fix.)
comment:14 in reply to: ↑ 13 ; follow-up: ↓ 15 Changed 8 years ago by
there is no point in requiring this mechanism
Depends what you mean. The path-rewriting is needed to be able to relocate the Sage install tree.
Let me repeat this: By special-casing ipython
you are achieving absolutely nothing: the "hack" in sage-location
still exists and you're just fooling the doctest.
A proper fix for the "hack" would fix all Python scripts, not just this one. What's the point of this special case?
comment:15 in reply to: ↑ 14 ; follow-up: ↓ 16 Changed 8 years ago by
Replying to jdemeyer:
there is no point in requiring this mechanism
Depends what you mean. The path-rewriting is needed to be able to relocate the Sage install tree.
What i mean is, there is no point in requiring the mechanism for ipython
. Most (maybe not all) cases are like this.
Let me repeat this: By special-casing
ipython
you are achieving absolutely nothing: the "hack" insage-location
still exists and you're just fooling the doctest.
there's a proper doctest in #15146, that does not rely on a wrong ipython installation.
A proper fix for the "hack" would fix all Python scripts, not just this one.
Yes.
What's the point of this special case?
I cannot fix all at once. But before the line has been merged, I will certainly not paste it into the other spkg-install
files.
comment:16 in reply to: ↑ 15 ; follow-up: ↓ 17 Changed 8 years ago by
Replying to felixs:
What's the point of this special case?
I cannot fix all at once.
You just gave a very good argument for keeping the sage-location
"hack": it does rewrite all the paths at once without trouble. What's wrong with that?
I think you still need to explain what the problem is that this ticket is trying to fix, I really don't understand it.
comment:17 in reply to: ↑ 16 Changed 8 years ago by
Replying to jdemeyer:
Replying to felixs:
What's the point of this special case?
I cannot fix all at once.
You just gave a very good argument for keeping the
sage-location
"hack":
No, I'm trying to make Sage a sane place. Something like this takes time.
it does rewrite all the paths at once without trouble. What's wrong with that?
It doesn't do anything useful in *most* cases. If there were no doctest looking at ipython i wouldn't even have noticed. A hack should always be restricted to things that require it. This is pretty similar to the problem about static atlas libraries (#15045): Its always better to not break/complicate things for *all* users just because one CPU/platform is broken/limitied/buggy.
I think you still need to explain what the problem is that this ticket is trying to fix, I really don't understand it.
Here we go, a more formal argument. read http://en.wikipedia.org/wiki/Encapsulation_(object-oriented_programming). There is a file that takes care of the contents of a package (spkg-install
). Nothing beyond that is required. Other routines/programs that mess wich package contents, like sage-location
does, violate encapsulation. Encapsulation, in general, is useful to keep things comprehensible and simple.
comment:18 Changed 7 years ago by
- Milestone changed from sage-6.0 to sage-6.1
comment:19 Changed 7 years ago by
- Milestone changed from sage-6.1 to sage-6.2
comment:20 Changed 7 years ago by
- Milestone changed from sage-6.2 to sage-6.3
comment:21 Changed 7 years ago by
- Milestone changed from sage-6.3 to sage-6.4
comment:22 Changed 6 years ago by
I think we can close this as duplicate (sort of) of #15146.
comment:23 Changed 6 years ago by
- Milestone changed from sage-6.4 to sage-duplicate/invalid/wontfix
- Reviewers set to Jeroen Demeyer
- Status changed from needs_info to positive_review
comment:24 Changed 6 years ago by
- Resolution set to duplicate
- Status changed from positive_review to closed
The "hack" you mention has nothing at all to do with
ipython
. Whatsage-location
does affects every Python script in$SAGE_LOCAL/bin
, so I don't see what this patch is trying to accomplish.