Opened 7 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) Commit: 9c372862bc86c6aa09b9fbf94711405d716abff7
Dependencies: #15146 Stopgaps:

Description (last modified by jdemeyer)

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 7 years ago by felixs

  • Status changed from new to needs_review

comment:2 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-5.12 to sage-6.0

comment:3 Changed 7 years ago by jondo

  • Cc jondo added

comment:4 Changed 7 years ago by jdemeyer

  • Status changed from needs_review to needs_work

The "hack" you mention has nothing at all to do with ipython. What sage-location does affects every Python script in $SAGE_LOCAL/bin, so I don't see what this patch is trying to accomplish.

comment:5 follow-up: Changed 7 years ago by felixs

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 7 years ago by jdemeyer

Replying to felixs:

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?

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: Changed 7 years ago by felixs

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: Changed 7 years ago by jdemeyer

  • 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 7 years ago by jdemeyer

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 7 years ago by felixs

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" in sage-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 7 years ago by felixs

  • Dependencies set to #15146
  • Status changed from needs_work to needs_review

comment:12 Changed 7 years ago by jdemeyer

  • 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: Changed 7 years ago by felixs

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: Changed 7 years ago by 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.

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: Changed 7 years ago by felixs

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" in sage-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: Changed 7 years ago by 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": 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.

Last edited 7 years ago by jdemeyer (previous) (diff)

comment:17 in reply to: ↑ 16 Changed 7 years ago by felixs

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 vbraun_spam

  • Milestone changed from sage-6.0 to sage-6.1

comment:19 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:20 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:21 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:22 Changed 6 years ago by jdemeyer

I think we can close this as duplicate (sort of) of #15146.

comment:23 Changed 6 years ago by jdemeyer

  • Authors Felix Salfelder deleted
  • 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 vbraun

  • Resolution set to duplicate
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.