Opened 6 years ago

Closed 5 years ago

#15596 closed defect (fixed)

Sdist fails with capital-P illow

Reported by: vbraun Owned by:
Priority: blocker Milestone: sage-6.1
Component: scripts Keywords:
Cc: Merged in:
Authors: Volker Braun, Jeroen Demeyer Reviewers: R. Andrew Ohana
Report Upstream: N/A Work issues:
Branch: u/jdemeyer/ticket/15596 (Commits) Commit: c7c0106e5c7c984e36abd7083b109b4be82610b6
Dependencies: Stopgaps:

Description (last modified by vbraun)

The sage-sdist script tries to download pillow, but correct tarball name is Pillow.

Change History (19)

comment:1 Changed 6 years ago by vbraun

  • Component changed from PLEASE CHANGE to scripts
  • Status changed from new to needs_review
  • Type changed from PLEASE CHANGE to defect

comment:2 Changed 6 years ago by vbraun

  • Branch set to u/vbraun/sdist_fails_with_capital_p_illow
  • Commit set to 20c0c35aa524abaaba78b729d86c4de5d5c1ecbe
  • Priority changed from major to blocker

New commits:

20c0c35correct the tarball name in sage -sdist
321a9adMerge remote-tracking branch 'trac/u/ohanar/pillow' into t/15596/sdist_fails_with_capital_p_illow
f005905pillow: depends on setuptools
adc90cePillow -> pillow
8987381remove PIL (it has been replaced by Pillow)
2c055bdPillow: fix patch to work against 2.2.2
6690d97doc: remove now unused environment variables
3778f42Pillow: re-resolve #9864
6df0adbreplace PIL with Pillow

comment:3 Changed 6 years ago by vbraun

  • Authors set to Volker Braun

comment:4 Changed 6 years ago by vbraun

Only the topmost commit matters, the rest is part of #15539 and already merged (but not released since this blocks release)

comment:5 Changed 6 years ago by vbraun

  • Description modified (diff)

comment:6 Changed 6 years ago by vbraun

  • Description modified (diff)

comment:7 Changed 6 years ago by ncohen

HMmmm... I don't get what all this does (and the last commit of #15539 is f005905, which means that this ticket contains 6 commits), but it looks like they have all been written by Andrew. Soooo if you can review them by yourself it seems ^^;

Nathann

comment:8 Changed 6 years ago by vbraun

This ticket contains (in reverse order):

  • the 7 commits from #15539, ending with f005905
  • a merge commit so I can actually trigger the problem in this branch
  • the fix for the bug in the topmost commit.

comment:9 Changed 5 years ago by jdemeyer

Would it not be sufficient to revert adc90cead77a91f17ea85dcbbfe77d5aba881465?

comment:10 Changed 5 years ago by vbraun

Well in #15539 we decided to (at least for now) stick with the current naming convention where the subdirectories of build/pkgs are all lower-case.

But regardless of that, we should use the tarball name from the checksums.ini file and not blindly try combinations of basename + extension. So even if we, in the future, decide to rename the directories under build/pkgs we should still merge this ticket.

comment:11 Changed 5 years ago by jdemeyer

  • Branch changed from u/vbraun/sdist_fails_with_capital_p_illow to u/jdemeyer/ticket/15596
  • Created changed from 12/27/13 14:22:33 to 12/27/13 14:22:33
  • Modified changed from 12/29/13 03:49:46 to 12/29/13 03:49:46

comment:12 Changed 5 years ago by jdemeyer

  • Authors changed from Volker Braun to Volker Braun, Jeroen Demeyer
  • Commit changed from 20c0c35aa524abaaba78b729d86c4de5d5c1ecbe to c7c0106e5c7c984e36abd7083b109b4be82610b6

I think this is a much better solution.


New commits:

c7c0106sage-sdist: copy upstream tarballs using sage-spkg

comment:13 Changed 5 years ago by mraum

  • Modified changed from 12/29/13 10:26:58 to 12/29/13 10:26:58
  • Status changed from needs_review to positive_review

comment:14 follow-up: Changed 5 years ago by mraum

  • Status changed from positive_review to needs_review

I didn't see anything bad about Volker's solution. But I have to have a look at Jereon's changes.

Btw: why didn't the dev-script notice that Jereon changed the commit field just before I set the ticket to positive review?

comment:15 in reply to: ↑ 14 Changed 5 years ago by jdemeyer

Replying to mraum:

I didn't see anything bad about Volker's solution.

It's reinventing part of the sage-spkg wheel, my solution instead directly uses sage-spkg to copy the files.

comment:16 Changed 5 years ago by mraum

That absolutely makes sense. Unfortunately, I won't have time to look at this until the second week of January. So if Volker has a look at Jereon's changes, he can consider his part of the patch as positively reviewed. That's all I can do for the moment.

comment:17 Changed 5 years ago by ohanar

  • Milestone changed from sage-6.1 to sage-duplicate/invalid/wontfix
  • Status changed from needs_review to positive_review

Reviewed as part of #15580.

comment:18 Changed 5 years ago by jdemeyer

  • Milestone changed from sage-duplicate/invalid/wontfix to sage-6.1
  • Reviewers set to R. Andrew Ohana

Given that it is reviewed and should be merged and the tickets even have different authors, I don't think sage-duplicate/invalid/wontfix is right.

comment:19 Changed 5 years ago by vbraun

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