Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.debian.maint.python > #8537 > unrolled thread

python-social-auth 0.2.19-1 review

Back to article view | Back to linux.debian.maint.python

Contents

  python-social-auth 0.2.19-1 review Tiago Ilieve <tiago.myhro@gmail.com> - 2016-05-17 07:40 +0200
    Re: python-social-auth 0.2.19-1 review Michael Fladischer <michael@fladi.at> - 2016-05-19 13:50 +0200
      Re: python-social-auth 0.2.19-1 review Tiago Ilieve <tiago.myhro@gmail.com> - 2016-05-19 18:40 +0200
        Re: python-social-auth 0.2.19-1 review Dmitry Shachnev <mitya57@debian.org> - 2016-05-21 10:50 +0200
          Re: python-social-auth 0.2.19-1 review Tiago Ilieve <tiago.myhro@gmail.com> - 2016-05-24 12:30 +0200
            Re: python-social-auth 0.2.19-1 review Tiago Ilieve <tiago.myhro@gmail.com> - 2016-05-30 08:20 +0200
              Re: python-social-auth 0.2.19-1 review Dmitry Shachnev <mitya57@debian.org> - 2016-06-01 13:50 +0200
                Re: python-social-auth 0.2.19-1 review Tiago Ilieve <tiago.myhro@gmail.com> - 2016-06-08 22:40 +0200

#8537 — python-social-auth 0.2.19-1 review

Hi DPMT,

In my first task as a member of the Debian Python Modules Team, I've
prepared an upload of "python-social-auth"[1]. It was updated to a
newer upstream release (0.2.13 to 0.2.19) and some fixes were also
done.

Is there anyone here able to review/sponsor those changes?

Regards,
Tiago.

[1]: https://anonscm.debian.org/git/python-modules/packages/python-social-auth.git

-- 
Tiago "Myhro" Ilieve
Blog: https://blog.myhro.info/
GitHub: https://github.com/myhro
LinkedIn: https://br.linkedin.com/in/myhro
Montes Claros - MG, Brasil

[toc] | [next] | [standalone]

#8539

[Multipart message — attachments visible in raw view] — view raw

Hi Tiago,

On Tue, 2016-05-17 at 02:14 -0300, Tiago Ilieve wrote:
> In my first task as a member of the Debian Python Modules Team, I've
> prepared an upload of "python-social-auth"[1]. It was updated to a
> newer upstream release (0.2.13 to 0.2.19) and some fixes were also
> done.
> 
> Is there anyone here able to review/sponsor those changes?

my quick review after building it:

- lintian complains about site/js/bootstrap.min.js, AFAIKT the "site"
folder  contains the project's website. Maybe you would like to remove
it by repacking the source tarball.
- You could shorten "python-all (>= 2.7~)" from Build-Depends to
"python-all".
- It's more accurate to use "Expat" instead of "MIT" in d/copyright.
- Both binary packages ship the documentation. While it's only a few KB
it would be an option to move the documentation to a separate binary
package.

Cheers and thanks for your work!
-- 
Michael Fladischer
Fladi.at

[toc] | [prev] | [next] | [standalone]

#8540

Hi Michael,

On 19 May 2016 at 08:41, Michael Fladischer <michael@fladi.at> wrote:
> my quick review after building it:
>
> - lintian complains about site/js/bootstrap.min.js, AFAIKT the "site"
> folder  contains the project's website. Maybe you would like to remove
> it by repacking the source tarball.

There's a patch[1] adding sources for bootstrap CSS/JS minified files,
but this doesn't makes much sense, right? They aren't included in
binary, nor the orig source tarball. I can repack it as a
DSFG-compatible tarball, but would like to receive a second opinion
about the mentioned patch, confirming if it is really useless.

> - You could shorten "python-all (>= 2.7~)" from Build-Depends to
> "python-all".

Noticed but somehow forgot about it later. Done[2].

> - It's more accurate to use "Expat" instead of "MIT" in d/copyright.

I respectfully disagree with you at this point, as I had already
talked about it on "debian-mentors" last month[3]. In this case
there's even an additional detail: the copyrighted files specifically
uses the "MIT" name for the license. Using a different name under
"debian/copyright" would be an inconsistency.

> - Both binary packages ship the documentation. While it's only a few KB
> it would be an option to move the documentation to a separate binary
> package.

Nice catch. I've added a separate binary package for the
documentation[4] and built it as HTML instead of text (Debian Policy
Manual, §12.4 Preferred documentation formats[5]).

> Cheers and thanks for your work!

You're welcome. Thanks a lot for your review. :-)

[1]: https://anonscm.debian.org/git/python-modules/packages/python-social-auth.git/tree/debian/patches/0001-append-uncompressed-bootstrap-as-its-source.patch?id=f94ec2c
[2]: https://anonscm.debian.org/git/python-modules/packages/python-social-auth.git/commit/?id=f4543a1
[3]: https://lists.debian.org/debian-mentors/2016/04/msg00060.html
[4]: https://anonscm.debian.org/git/python-modules/packages/python-social-auth.git/commit/?id=f94ec2c
[5]: https://www.debian.org/doc/debian-policy/ch-docs.html#s12.4

-- 
Tiago "Myhro" Ilieve
Blog: https://blog.myhro.info/
GitHub: https://github.com/myhro
LinkedIn: https://br.linkedin.com/in/myhro
Montes Claros - MG, Brasil

[toc] | [prev] | [next] | [standalone]

#8543

[Multipart message — attachments visible in raw view] — view raw

Hi Tiago,

On Thu, May 19, 2016 at 01:19:46PM -0300, Tiago Ilieve wrote:
>> - lintian complains about site/js/bootstrap.min.js, AFAIKT the "site"
>> folder  contains the project's website. Maybe you would like to remove
>> it by repacking the source tarball.
>
> There's a patch[1] adding sources for bootstrap CSS/JS minified files,
> but this doesn't makes much sense, right? They aren't included in
> binary, nor the orig source tarball. I can repack it as a
> DSFG-compatible tarball, but would like to receive a second opinion
> about the mentioned patch, confirming if it is really useless.

I think it's better to put the missing sources in debian/missing-sources/
directory rather than patching them in.

(That is also suggested by the Lintian error description[1], and should
make that error disappear.)

[1] https://lintian.debian.org/tags/source-is-missing.html

>> - It's more accurate to use "Expat" instead of "MIT" in d/copyright.
>
> I respectfully disagree with you at this point, as I had already
> talked about it on "debian-mentors" last month[3]. In this case
> there's even an additional detail: the copyrighted files specifically
> uses the "MIT" name for the license. Using a different name under
> "debian/copyright" would be an inconsistency.

According to the copyright format specification[2]:

| There are many versions[3] of the MIT license. Please use Expat[4] instead,
| when it matches.

[2] https://www.debian.org/doc/packaging-manuals/copyright-format/1.0/
[3] https://en.wikipedia.org/wiki/MIT_License#Various_versions
[4] http://www.jclark.com/xml/copying.txt

--
Dmitry Shachnev

[toc] | [prev] | [next] | [standalone]

#8546

Hi Dmitry,

On 21 May 2016 at 05:47, Dmitry Shachnev <mitya57@debian.org> wrote:
> I think it's better to put the missing sources in debian/missing-sources/
> directory rather than patching them in.
>
> (That is also suggested by the Lintian error description[1], and should
> make that error disappear.)

The pedantic warning is actually
"source-contains-prebuilt-javascript-object", not a
"source-is-missing" error. This solution won't apply to this case.

Looking through the files, there are some interesting details:

- "0001-append-uncompressed-bootstrap-as-its-source.patch" won't
apply. The uncompressed JS/CSS files were previously added[1] directly
to the upstream "site/" folder. Does this configures as a Debian
Policy violation?
- The "site/" folder isn't included (or used at all) in the binary
packages. What happened is that upstream decided to include the
project website in the same repository. Should we really worry about
its contents?

Thinking about this, the best solution seems to be drop the patch and
remove the "site/" folder, repacking it as a DSFG-compatible tarball.
But if there isn't a strictly legal requirement (e.g. to include
sources for files that aren't distributed in binary form), I'm not
sure this worth the effort.

> According to the copyright format specification[2]:
>
> | There are many versions[3] of the MIT license. Please use Expat[4] instead,
> | when it matches.

At first I thought this was clarifying and confusing at the same time,
but now I got it. It's not that the "MIT License" is a wrong name for
it, but a non-ambiguous one. Fixed[2] and sorry for insisting on it. I
hadn't figured that "MIT" wasn't a valid short name under the
machine-readable format.

Regards,
Tiago.

[1]: https://anonscm.debian.org/git/python-modules/packages/python-social-auth.git/commit/?id=71c4050
[2]: https://anonscm.debian.org/git/python-modules/packages/python-social-auth.git/commit/?id=1a445da

-- 
Tiago "Myhro" Ilieve
Blog: https://blog.myhro.info/
GitHub: https://github.com/myhro
LinkedIn: https://br.linkedin.com/in/myhro
Montes Claros - MG, Brasil

[toc] | [prev] | [next] | [standalone]

#8551

Dmitry and Michael,

On 24 May 2016 at 07:08, Tiago Ilieve <tiago.myhro@gmail.com> wrote:
> Thinking about this, the best solution seems to be drop the patch and
> remove the "site/" folder, repacking it as a DSFG-compatible tarball.
> But if there isn't a strictly legal requirement (e.g. to include
> sources for files that aren't distributed in binary form), I'm not
> sure this worth the effort.

At first I thought this would be hard[1], but following the example
from "python-docutils"[2][3] proved to be quite straightforward. I've
pushed the changes[4], but there are two related things that I didn't
figured how to do properly:

- Commit the repackaged tarball contents in the "upstream" branch and
create a tag named with "+dfsg" suffix. Is "gbp import-orig" able to
do this by itself?
- Add "pristine-tar" data for the repackaged tarball. This could
probably be done when solving the previous question.

If you guys can kindly take a look, I'd appreciate.

Regards,
Tiago.

[1]: https://wiki.debian.org/BenFinney/software/repack
[2]: https://sources.debian.net/src/python-docutils/0.12%2Bdfsg-1/debian/copyright/
[3]: https://sources.debian.net/src/python-docutils/0.12%2Bdfsg-1/debian/watch/
[4]: https://anonscm.debian.org/git/python-modules/packages/python-social-auth.git/commit/?id=65fd5ed

-- 
Tiago "Myhro" Ilieve
Blog: https://blog.myhro.info/
GitHub: https://github.com/myhro
LinkedIn: https://br.linkedin.com/in/myhro
Montes Claros - MG, Brasil

[toc] | [prev] | [next] | [standalone]

#8557

[Multipart message — attachments visible in raw view] — view raw

Hi Tiago,

On Mon, May 30, 2016 at 03:00:05AM -0300, Tiago Ilieve wrote:
> At first I thought this would be hard[1], but following the example
> from "python-docutils"[2][3] proved to be quite straightforward. I've
> pushed the changes[4], but there are two related things that I didn't
> figured how to do properly:
>
> - Commit the repackaged tarball contents in the "upstream" branch and
> create a tag named with "+dfsg" suffix. Is "gbp import-orig" able to
> do this by itself?
> - Add "pristine-tar" data for the repackaged tarball. This could
> probably be done when solving the previous question.

Usually one would do both things using:

  git-dpm import-new-upstream --pristine-tar-commit /path/to/tarball

In your case .git-dpm was inconsistent with upstream branch, so I had to
pass the additional --use-strange-upstream-branch argument to the above
command.

Please use the instructions at https://wiki.debian.org/Python/GitPackaging
next time when i.e. importing a new upstream version.

> If you guys can kindly take a look, I'd appreciate.

I have also pushed some packaging simplifications to Git, and also fixed
the KGB hook (s/sphinx/python-social-auth/).

I will upload the package later today.

Also, I noticed that the comment about disabling tests in debian/rules is
out-of-date: the *requirements*.txt files depend on nose >= 1.2.1, which
should be OK (though there are hard dependencies on version numbers for
other packages).

It would be nice to get the tests run during build again, if possible.

--
Dmitry Shachnev

[toc] | [prev] | [next] | [standalone]

#8572

Hi Dmitry,

(Sorry for taking so long to reply.)

On 1 June 2016 at 08:40, Dmitry Shachnev <mitya57@debian.org> wrote:
> Usually one would do both things using:
>
>   git-dpm import-new-upstream --pristine-tar-commit /path/to/tarball
>
> In your case .git-dpm was inconsistent with upstream branch, so I had to
> pass the additional --use-strange-upstream-branch argument to the above
> command.
>
> Please use the instructions at https://wiki.debian.org/Python/GitPackaging
> next time when i.e. importing a new upstream version.

Had imported the new upstream version using "gbp". I'll use "git-dpm"
from now on.

> I have also pushed some packaging simplifications to Git, and also fixed
> the KGB hook (s/sphinx/python-social-auth/).

I totally forgot about passing "sphinxdoc" to "dh". Thanks for taking
care of this.

> I will upload the package later today.

It is in testing already. Thank you very much! :-)

> Also, I noticed that the comment about disabling tests in debian/rules is
> out-of-date: the *requirements*.txt files depend on nose >= 1.2.1, which
> should be OK (though there are hard dependencies on version numbers for
> other packages).
>
> It would be nice to get the tests run during build again, if possible.

I've tried to run the tests and looks like there's a problem with the
dependencies:

- "requirements.txt" specifies[1] "requests-oauthlib>=0.4.0" but when
running the tests on Python 2.7, it asks for
"requests-oauthlib>=0.6.1", which is specified[2] in
"requirements-python3.txt". Not sure why one superseded the other.
- "python-saml==2.1.3" is specified[3] in
"social/tests/requirements.txt", which suggests that it is a test-only
dependency. But under "social/backends/saml.py" there are a few
imports[4] from "onelogin.saml2.*", which makes me think that there's
a missing runtime dependency.

Am I misunderstanding something here or there's really a confusion
regarding the dependencies?

Regards,
Tiago.

[1]: https://anonscm.debian.org/git/python-modules/packages/python-social-auth.git/tree/requirements.txt?id=b2df566#n4
[2]: https://anonscm.debian.org/git/python-modules/packages/python-social-auth.git/tree/requirements-python3.txt?id=b2df566#n4
[3]: https://anonscm.debian.org/git/python-modules/packages/python-social-auth.git/tree/social/tests/requirements.txt?id=b2df566#n9
[4]: https://anonscm.debian.org/git/python-modules/packages/python-social-auth.git/tree/social/backends/saml.py?id=b2df566#n10

-- 
Tiago "Myhro" Ilieve
Blog: https://blog.myhro.info/
GitHub: https://github.com/myhro
LinkedIn: https://br.linkedin.com/in/myhro
Montes Claros - MG, Brasil

[toc] | [prev] | [standalone]

Back to top | Article view | linux.debian.maint.python

csiph-web