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

Upgrade patchbot to 2.6.1 as a system package #20736

Closed
fchapoton opened this issue May 31, 2016 · 184 comments
Closed

Upgrade patchbot to 2.6.1 as a system package #20736

fchapoton opened this issue May 31, 2016 · 184 comments

Comments

@fchapoton
Copy link
Contributor

The patchbot is currently installing in a very not kosher way, as explained in

sagemath/sage-patchbot#79

This ticket will change completely the installation mode. The patchbot will become a python package, to be installed using pip in
the system python as explained below.

This ticket also updates the patchbot, in particular to cope with the new https trac.

New tarball for patchbot: http://www-irma.u-strasbg.fr/~chapoton/sage-patchbot-2.6.1.tar.gz

installation (two equivalent methods at your disposal):

  1. Download the tarball and then
pip install --user sage-patchbot-2.6.1.tar.gz
  1. (recommended)
pip install --user git+https://github.com/sagemath/sage-patchbot.git@2.6.1

There are now two possible ways to launch the patchbot:
1.

python -m sage_patchbot.patchbot --sage-root=XXXX

or
2. Checkout the branch here and run

make && ./sage --patchbot

Depends on #22473

CC: @embray @jdemeyer

Component: packages: optional

Keywords: patchbot

Author: Frédéric Chapoton

Branch/Commit: c9c4101

Reviewer: Jeroen Demeyer

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

@fchapoton

This comment has been minimized.

@fchapoton
Copy link
Contributor Author

comment:2

I tried and failed so far. Help is required, please.


New commits:

a827087trac 20736 first try, failing

@fchapoton
Copy link
Contributor Author

Branch: public/20736

@fchapoton
Copy link
Contributor Author

Commit: a827087

@embray
Copy link
Contributor

embray commented Jun 1, 2016

comment:3

Not related to installation problem, but in the dependencies file you should put

$(PYTHON) | setuptools

That is, if setuptools is reinstalled it's not necessary to reinstall a Python package that happens to use it. But it is a prerequisite to installing.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 1, 2016

Branch pushed to git repo; I updated commit sha1. New commits:

c9907eftrac 20736 change in dependencies

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 1, 2016

Changed commit from a827087 to c9907ef

@embray
Copy link
Contributor

embray commented Jun 1, 2016

comment:5

BTW, when building a source package of a Python project, use ./setup.py sdist to make the tarball (also make sure the tarball contains all the files it should contain, and none that it shouldn't contain--you might have to edit a MANIFEST.in file to adjust it a la https://docs.python.org/2/distutils/sourcedist.html#specifying-the-files-to-distribute)

@fchapoton

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 1, 2016

Changed commit from c9907ef to f3839fd

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 1, 2016

Branch pushed to git repo; I updated commit sha1. New commits:

f3839fdanother try, failing the same way

@fchapoton
Copy link
Contributor Author

comment:8

I am making some slow progress.

@fchapoton

This comment has been minimized.

@jdemeyer
Copy link

jdemeyer commented Jun 1, 2016

comment:10

There is a fundamental problem here, mentioned also at sagemath/sage-patchbot#79

We cannot use the Sage Python for the patchbot, since the patchbot might reinstall Python. So I don't know what the best solution is, but installing it "the usual way" is not the right thing to do here.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 1, 2016

Branch pushed to git repo; I updated commit sha1. New commits:

160332cfixing the spkg-install script

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 1, 2016

Changed commit from f3839fd to 160332c

@embray
Copy link
Contributor

embray commented Jun 2, 2016

comment:12

So it's a build dep for Sage and should be installed "the usual way" using the system Python.

@jdemeyer
Copy link

jdemeyer commented Jun 2, 2016

comment:13

Replying to @embray:

So it's a build dep for Sage and should be installed "the usual way" using the system Python.

I don't get why you say a "build dep", but I agree with the second part.

@fchapoton
Copy link
Contributor Author

comment:14

I changed the name of the package back to sage-patchbot. But I am little bit stuck here.

It kind of worked with the package name "patchbot", but it was installed in SAGE_LOCAL.

@embray
Copy link
Contributor

embray commented Jun 2, 2016

comment:15

Replying to @jdemeyer:

Replying to @embray:

So it's a build dep for Sage and should be installed "the usual way" using the system Python.

I don't get why you say a "build dep", but I agree with the second part.

"build dep" is probably the wrong description--but I just meant a developer tool for development of Sage, outside the sage distribution.

@embray
Copy link
Contributor

embray commented Jun 2, 2016

comment:16

Replying to @fchapoton:

I changed the name of the package back to sage-patchbot. But I am little bit stuck here.

It kind of worked with the package name "patchbot", but it was installed in SAGE_LOCAL.

To be clear, this is a case where the term "package" is often overloaded in Python:

  • Here the package, as in the name of what you import in Python is patchbot

  • The name in the setup.py is different from the package name, and as sometimes referred to as the "distribution name" or the "PyPI name" to reduce confusion. Often the package name and the distribution name are the same, but they don't have to be. Either way it shouldn't be relevant here.

I think the main takeaway here is that the patchbot should not be a Sage spkg at all. It's not a part of sage, the distribution. It's a tool for testing sage the distribution and as such should be entirely external to it.

So to install the patchbot on say a Linux system one would use sudo pip install <path/to/tarball> or even just sudo pip install sage-patchbot if the latest version is uploaded to PyPI.

Alternatively this can be done within a virtualenv if we prefer to avoid the system install.

@fchapoton
Copy link
Contributor Author

New commits:

eed116fMerge branch 'public/20736' into 7.3.b4
228842bupdate of patchbot to 2.6.0

@fchapoton
Copy link
Contributor Author

Changed commit from 160332c to 228842b

@fchapoton

This comment has been minimized.

@fchapoton fchapoton changed the title make patchbot a correct python package upgrade patchbot to 2.6.0 (compatible with https trac) Jun 22, 2016
@fchapoton
Copy link
Contributor Author

comment:18

This is becoming urgent, to cope with https trac.

I tried to go back to the previous way but this is not working. It complains about

sage -patchbot --ticket=20862
Traceback (most recent call last):
  File "sage/local/bin/patchbot/patchbot.py", line 63, in <module>
    from .trac import scrape, pull_from_trac, TracServer, Config
ValueError: Attempted relative import in non-package

I need help. Should I just remove all relative imports newly introduced ?? should I keep the strange source dir of patchbot inside the local/bin directory ??

@jdemeyer jdemeyer modified the milestones: sage-7.4, sage-7.6 Jan 5, 2017
@sagetrac-tmonteil
Copy link
Mannequin

sagetrac-tmonteil mannequin commented Feb 28, 2017

comment:144

Since this ticket seems not to reach a consensus, what about splitting it into two part:

  • remove patchbot optional spkg, and deprecate the related sage -patchbot command (this can be done for next beta)
  • discuss and write a convenient alternative command or documentation to run the patchbot (this seems the blocking point and might require more time)

I can volonteer to implement the first point on top of the latest beta ;)

@jdemeyer
Copy link

comment:145

Replying to @sagetrac-tmonteil:

  • remove patchbot optional spkg, and deprecate the related sage -patchbot command (this can be done for next beta)
  • discuss and write a convenient alternative command or documentation to run the patchbot (this seems the blocking point and might require more time)

Doesn't "deprecate the related sage -patchbot command" relate more to the second point instead of the first?

So I would rather split up as

  1. remove patchbot optional spkg
  2. discuss how to run the patchbot and what to do with sage --patchbot

@sagetrac-tmonteil
Copy link
Mannequin

sagetrac-tmonteil mannequin commented Feb 28, 2017

comment:146

OK, see #22473.

Replying to @jdemeyer:

Doesn't "deprecate the related sage -patchbot command" relate more to the second point instead of the first?

So I would rather split up as

  1. remove patchbot optional spkg
  2. discuss how to run the patchbot and what to do with sage --patchbot

OK, I did not deprecate the command, but since the current sage -patchbot command does not work anyway, i replaced it with a short sentence pointing to https://wiki.sagemath.org/buildbot#Installing_the_patchbot (which are the most up-to-date instructions). It is just a suggestion until the current ticket get fixed, i can undo the last commit about the sage -patchbot command if required.

@jdemeyer
Copy link

comment:147

Replying to @sagetrac-tmonteil:

It is just a suggestion until the current ticket get fixed

For the record: in my opinion, nothing needs to be fixed since nothing is broken.

@jdemeyer
Copy link

comment:148

Replying to @sagetrac-tmonteil:

https://wiki.sagemath.org/buildbot#Installing_the_patchbot (which are the most up-to-date instructions). It is just a suggestion until the current ticket get fixed

It's also an invitation to move the current discussion on this ticket to an edit war on that wiki page, so I am not so happy with that.

@sagetrac-tmonteil
Copy link
Mannequin

sagetrac-tmonteil mannequin commented Feb 28, 2017

comment:149

Replying to @jdemeyer:

Replying to @sagetrac-tmonteil:

https://wiki.sagemath.org/buildbot#Installing_the_patchbot (which are the most up-to-date instructions). It is just a suggestion until the current ticket get fixed

It's also an invitation to move the current discussion on this ticket to an edit war on that wiki page, so I am not so happy with that.

OK, i removed that commit, the goal is to get rid of this broken optional package earlier, without entering the picky part of the current ticket.

@embray
Copy link
Contributor

embray commented Mar 1, 2017

comment:150

I think IIRC I was fine with keeping sage -patchbot as a shortcut for "run patchbot with the --sage-root option set from this sage). I think that makes sense, and I think that's what Jeroen was trying to do originally. I just thought the solution was overcomplicated.

I don't know why we're talking about python -m anything. When you install the sage-patchbot Python package (which Frédéric did a great job putting together at my urging) it installs a patchbot script. If you run that it just works, you just have to supply the correct --sage-root argument.

@jdemeyer
Copy link

jdemeyer commented Mar 1, 2017

comment:151

What I want in particular is to install the patchbot within the Sage environment and have sage --patchbot recognize that.

Of course, I am totally open to supporting any other use cases too (unlike some other people who explicitly do not want to suppport my use case).

@embray
Copy link
Contributor

embray commented Mar 1, 2017

comment:152

Was there some reason that wouldn't work? That is, if you ran sage -pip install sage-patchbot? Then, in principle, it would work if you ran sage -sh -c patchbot though I guess if $SAGE_LOCAL/bin/patchbot exists then sage -patchbot can be a shortcut for that. I'm just not sure why you'd need or want to do that but I don't oppose it.

@jdemeyer
Copy link

jdemeyer commented Mar 1, 2017

Dependencies: #22473

@jdemeyer
Copy link

jdemeyer commented Mar 1, 2017

comment:154

Replying to @embray:

I don't know why we're talking about python -m anything. When you install the sage-patchbot Python package (which Frédéric did a great job putting together at my urging) it installs a patchbot script.

...in a directory which might not be the default $PATH (especially in the case of a --user install). So there are cases where python -m sage_patchbot.patchbot works but running patchbot does not.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 1, 2017

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

b2810d6#22473 : remove the patchbot optional spkg.
537849ftrac 20736 first try, failing
b423b62Improve --patchbot command

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 1, 2017

Changed commit from d7c83f1 to b423b62

@jdemeyer
Copy link

jdemeyer commented Mar 1, 2017

comment:156

Rebased to #22473.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 1, 2017

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

c9c4101Improve --patchbot command

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 1, 2017

Changed commit from b423b62 to c9c4101

@jdemeyer
Copy link

jdemeyer commented Mar 1, 2017

comment:158

Let's start over...

@embray
Copy link
Contributor

embray commented Mar 3, 2017

comment:159

Replying to @jdemeyer:

Replying to @embray:

I don't know why we're talking about python -m anything. When you install the sage-patchbot Python package (which Frédéric did a great job putting together at my urging) it installs a patchbot script.

...in a directory which might not be the default $PATH (especially in the case of a --user install). So there are cases where python -m sage_patchbot.patchbot works but running patchbot does not.

That would imply someone has their ~/.local/lib/pythonX.Y/site-packages on sys.path, but not ~/.local/bin in their $PATH. That can certainly happen (I think it is the default in most cases) but I would consider it a slight misconfiguration. It is also not a problem at all if using virtualenvs instead.

@jdemeyer
Copy link

jdemeyer commented Mar 3, 2017

comment:161

I wonder what made you change your mind...

This code (which has been there since [comment:101]) has been attacked by several people for 6 months and now it suddenly has positive_review?

@embray
Copy link
Contributor

embray commented Mar 3, 2017

comment:162

My feeling at this point is along the lines of "I'm not going to use it, but if it suits your needs then fine." :)

@vbraun
Copy link
Member

vbraun commented Mar 5, 2017

Changed branch from public/20736 to c9c4101

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

8 participants