Opened 10 years ago

Closed 10 years ago

#13497 closed defect (fixed)

Disable sage --info test for bdists

Reported by: jdemeyer Owned by: mvngu
Priority: blocker Milestone: sage-5.4
Component: doctest coverage Keywords:
Cc: Merged in: sage-5.4.beta2
Authors: Jeroen Demeyer, John Palmieri Reviewers: John Palmieri
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

The doctest introduced for sage --info at #12606 fails for bdist distributions because those don't ship spkgs.

Apply 13497_sage_info_doctest.patch to the Sage library

Attachments (3)

trac_13497-root.patch (921 bytes) - added by jhpalmieri 10 years ago.
root repo
trac_13497-script.patch (1.6 KB) - added by jhpalmieri 10 years ago.
scripts repo
13497_sage_info_doctest.patch (1.2 KB) - added by jdemeyer 10 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 10 years ago by jdemeyer

  • Priority changed from critical to blocker

comment:2 Changed 10 years ago by jhpalmieri

Here's another option: instead of disabling the test, sage-bdist can store the info in the placeholder spkg file, and sage-spkg can read it from there. See the attached patches. In sage-bdist, we could instead use sage -i --info ... instead of the code stolen from sage-spkg.

This isn't very well tested, but it's an idea.

Changed 10 years ago by jhpalmieri

root repo

Changed 10 years ago by jhpalmieri

scripts repo

comment:3 Changed 10 years ago by jdemeyer

Maybe better would be to create an actual spkg but only put SPKG.txt in it...

But my feeling is that we're overdoing things. I would still simply disable the test.

Changed 10 years ago by jdemeyer

comment:4 Changed 10 years ago by jdemeyer

  • Authors set to Jeroen Demeyer, John Palmieri
  • Description modified (diff)

comment:5 follow-up: Changed 10 years ago by jhpalmieri

Your patch fixes the test, which is good. I guess the only question is whether we want to fix sage --info ... for bdists? Or document that it doesn't work for them? (Or is that two questions?)

I think that rewriting sage-bdist to create tar files which contain only one file, SPKG.txt, might not be a bad idea. Running sage-bdist would take a little longer, but one more aspect of Sage would function for bdists. (I also don't like the fact that as sage-bdist currently works, every original spkg file is copied and then overwritten with the placeholder file; this wastes time and disk space.)

comment:6 in reply to: ↑ 5 Changed 10 years ago by jdemeyer

Replying to jhpalmieri:

I think that rewriting sage-bdist to create tar files which contain only one file, SPKG.txt, might not be a bad idea.

On the other hand, I always disliked the "placeholder" spkgs in bdists and would actually prefer not to ship them (i.e. have spkg/standard essentially empty, apart from deps). Introducing these was a kludge to fix a problem in newest_version (now fixed in spkg/install).

comment:7 Changed 10 years ago by jdemeyer

  • Status changed from new to needs_review

comment:8 follow-up: Changed 10 years ago by jhpalmieri

Since you brought up the topic of sage --info and bdists, I feel uncomfortable with a documented feature of Sage not working with bdists. What do you think? I'm almost willing to give this a positive review, but I'm uncomfortable...

comment:9 in reply to: ↑ 8 Changed 10 years ago by jdemeyer

Replying to jhpalmieri:

Since you brought up the topic of sage --info and bdists, I feel uncomfortable with a documented feature of Sage not working with bdists. What do you think?

I would argue that sage --info does work within bdists. The problem isn't sage --info, the problem is that we're shipping spkgs which aren't spkgs. With your reasoning, sage -i is also broken.

That's why I think it would be better to not ship the fake .spkg files anymore for bdists (as far as I can tell, they are not needed anymore): #13522

comment:10 Changed 10 years ago by jhpalmieri

  • Reviewers set to John Palmieri
  • Status changed from needs_review to positive_review

comment:11 Changed 10 years ago by jdemeyer

  • Merged in set to sage-5.4.beta2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.