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

Sdist fails with capital-P illow #15596

Closed
vbraun opened this issue Dec 27, 2013 · 21 comments
Closed

Sdist fails with capital-P illow #15596

vbraun opened this issue Dec 27, 2013 · 21 comments

Comments

@vbraun
Copy link
Member

vbraun commented Dec 27, 2013

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

Component: scripts

Author: Volker Braun, Jeroen Demeyer

Branch/Commit: u/jdemeyer/ticket/15596 @ c7c0106

Reviewer: R. Andrew Ohana

Issue created by migration from https://trac.sagemath.org/ticket/15596

@vbraun vbraun added this to the sage-6.1 milestone Dec 27, 2013
@vbraun
Copy link
Member Author

vbraun commented Dec 27, 2013

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

@vbraun
Copy link
Member Author

vbraun commented Dec 27, 2013

Commit: 20c0c35

@vbraun
Copy link
Member Author

vbraun commented Dec 27, 2013

@vbraun
Copy link
Member Author

vbraun commented Dec 27, 2013

Author: Volker Braun

@vbraun
Copy link
Member Author

vbraun commented Dec 27, 2013

comment:4

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

@vbraun

This comment has been minimized.

@vbraun

This comment has been minimized.

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Dec 27, 2013

comment:7

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

@vbraun
Copy link
Member Author

vbraun commented Dec 27, 2013

comment:8

This ticket contains (in reverse order):

@jdemeyer
Copy link

comment:9

Would it not be sufficient to revert adc90ce?

@vbraun
Copy link
Member Author

vbraun commented Dec 29, 2013

comment:10

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.

@jdemeyer
Copy link

@jdemeyer
Copy link

Changed author from Volker Braun to Volker Braun, Jeroen Demeyer

@jdemeyer
Copy link

Changed commit from 20c0c35 to c7c0106

@jdemeyer
Copy link

comment:12

I think this is a much better solution.


New commits:

c7c0106sage-sdist: copy upstream tarballs using sage-spkg

@sagetrac-mraum
Copy link
Mannequin

sagetrac-mraum mannequin commented Dec 29, 2013

comment:14

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?

@jdemeyer
Copy link

comment:15

Replying to @sagetrac-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.

@sagetrac-mraum
Copy link
Mannequin

sagetrac-mraum mannequin commented Dec 29, 2013

comment:16

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.

@ohanar
Copy link
Member

ohanar commented Jan 3, 2014

comment:17

Reviewed as part of #15580.

@ohanar ohanar removed this from the sage-6.1 milestone Jan 3, 2014
@jdemeyer
Copy link

jdemeyer commented Jan 3, 2014

comment:18

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.

@jdemeyer
Copy link

jdemeyer commented Jan 3, 2014

Reviewer: R. Andrew Ohana

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants