Works for me and I have no `python` command available so it must be using `python3` You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/2903
-- Commit Summary --
* <a href="https://github.com/geany/geany/pull/2903/commits/59677542aa88bad3d8fdc4e6c40049d37b3e95e8">Apply Debian patch for Python 3</a>
-- File Changes --
M m4/geany-gtkdoc-header.m4 (2)
-- Patch Links --
https://github.com/geany/geany/pull/2903.patch https://github.com/geany/geany/pull/2903.diff
Why is this change needed, what problem does it fix?
Not trying to run Python 2, see #2896
Hmmm, seems our Linux CI doesn't install lxml, but the windows does.
Not trying to run Python 2, see #2896
At the very least the commit message must explain this, ideally the PR summary as well.
Ahhh, s/b installing python3-lxml package I think, I'm not on my dev machine for a few days (which is why I made this by hand)
@kugel- I think the title referring to Python 3 and the patch changing a 2.7 to a 3 is pretty obvious what its doing.
What's the rationale for the patch, why is it necessary? Especially since Python 2 is not dead yet
Python 2 is dead and buried and unsupported. Its not even installed on most current systems.
If you put the same effort into the commit message that you put into the discussion we would be green.
@kugel- requested changes on this pull request.
@@ -21,7 +21,7 @@ AC_DEFUN([GEANY_CHECK_GTKDOC_HEADER],
[test "x$geany_enable_gtkdoc_header" != "xno"], [ dnl python - AM_PATH_PYTHON([2.7], [have_python=yes], [have_python=no]) + AM_PATH_PYTHON([3], [have_python=yes], [have_python=no])
I see what the patch does. But WHY?
Ahhh, s/b installing python3-lxml package I think, I'm not on my dev machine for a few days (which is why I made this by hand)
Yes. The following could fix the pipeline: ``` diff --git a/.travis.yml b/.travis.yml index 03c03e0c..ebb28ffc 100644 --- a/.travis.yml +++ b/.travis.yml @@ -16,10 +16,10 @@ install: - test -z "$MINGW" || sudo apt-get install -y mingw-w64-tools g++-mingw-w64-i686 gcc-mingw-w64-i686 binutils-mingw-w64-i686 # fix broken pkg-config-crosswrapper, see https://bugs.launchpad.net/ubuntu/+source/mingw-w64/+bug/1327242 - test -z "$MINGW" || sudo sed -e 's/PKG_CONFIG_PATH=/&$PKG_CONFIG_PATH:/' -i /usr/bin/i686-w64-mingw32-pkg-config - - sudo apt-get install -y python-docutils rst2pdf + - sudo apt-get install -y python3-docutils rst2pdf # try not to install doxygen-latex because we don't need it and it's huge - sudo apt-get install -y --no-install-recommends doxygen - - sudo apt-get install -y python-lxml + - sudo apt-get install -y python3-lxml before_script: - export CFLAGS="-g -O2 -Werror=pointer-arith -Werror=implicit-function-declaration" script: ```
@eht16 commented on this pull request.
@@ -21,7 +21,7 @@ AC_DEFUN([GEANY_CHECK_GTKDOC_HEADER],
[test "x$geany_enable_gtkdoc_header" != "xno"], [ dnl python - AM_PATH_PYTHON([2.7], [have_python=yes], [have_python=no]) + AM_PATH_PYTHON([3], [have_python=yes], [have_python=no])
I agree the commit message could be a bit clearer. But, @kugel-, I really don't understand why it isn't obvious that we want to use Python3 for build related scripts and to drop Python2 which *IS* dead, since almost two years now. Is there any reason why we should keep using Python2 for build related scripts?
@kugel- commented on this pull request.
@@ -21,7 +21,7 @@ AC_DEFUN([GEANY_CHECK_GTKDOC_HEADER],
[test "x$geany_enable_gtkdoc_header" != "xno"], [ dnl python - AM_PATH_PYTHON([2.7], [have_python=yes], [have_python=no]) + AM_PATH_PYTHON([3], [have_python=yes], [have_python=no])
No. I solely object to the lack of *any* background information in the commit message and PR summary. I couldn't care less about python2.
@elextr commented on this pull request.
@@ -21,7 +21,7 @@ AC_DEFUN([GEANY_CHECK_GTKDOC_HEADER],
[test "x$geany_enable_gtkdoc_header" != "xno"], [ dnl python - AM_PATH_PYTHON([2.7], [have_python=yes], [have_python=no]) + AM_PATH_PYTHON([3], [have_python=yes], [have_python=no])
Its explained in #2896, but I didn't link it because I didn't want to close it automatically since this PR only does one part of that issue.
As I said above, I can't access my development machine ATM, and although I had tested this there, I didn't have time to make the PR so I just made it "by hand" on Github from my tablet using the little editing pencil. Just to try to be helpful.
I am not sure if there is any way of adding the `travis.yml` changes to the branch that this created without access to "proper" git.
Since it looks like it will be longer than expected before I get home I guess this can just get left here as a reminder, unless the changes are made as part of any other PR addressing #2896 then it and the branch can be deleted.
@eht16 pushed 1 commit.
e88f9c264d6f08a11964b124ce553cdf71fe2c77 Use Python3 for build dependencies in CI
I've just edited `.travis.yml' in your branch (directly online via Github) to make the change and a new build is running.
Also changed the title to try to describe better what this is about. We should also squash the commits on merge and then set a commit message describing the change. This should address the mentioned formality issues.
I've just edited `.travis.yml' in your branch (directly online via Github) to make the change and a new build is running.
How did you do that? As I said above I hadn't found a way and didn't think it was possible.
I've just edited `.travis.yml' in your branch (directly online via Github) to make the change and a new build is running.
How did you do that? As I said above I hadn't found a way and didn't think it was possible.
I guess it's simpler than you expect :): I opened https://github.com/geany/geany/blob/elextr-patch-1/.travis.yml which is the file in your branch and then edited it (find the pencil button in the upper right corner in the header box of the file in the UI). This created a commit in your branch and then triggered the CI build. 
Back to the content: I'd be fine to have this merged for 1.38.
@eht16 Ahhhhhhhhhhhhhhhhhhhhhhhhhhh, I know the pencil, thats how I made the original, what I missed was selecting the branch on the dropdown on the left so it wanted to make a new PR all the time.
@kugel- does this address your requested changes?
@b4n and me were wondering whether the generated header file will be different from Python2 to Python3 while releasing 1.38.
So I gave it a try: - For Python 3 (tested in a container, started like this: `docker run --rm -it -v /tmp/shared:/shared debian:buster-slim`): ``` apt-get update && apt-get install -y --no-install-recommends git doxygen intltool libtool libgtk-3-dev python3-lxml python3-docutils rst2pdf build-essential git clone --depth=1 https://github.com/geany/geany /geany-test && cd /geany-test PYTHON=python3 ./autogen.sh --enable-api-docs --enable-gtkdoc-header make -C doc cp doc/geany-gtkdoc.h /shared/geany-gtkdoc-py3.h ```
- For Python 2 (tested in a container, started like this: `docker run --rm -it -v /tmp/shared:/shared debian:buster-slim`): ``` apt-get update && apt-get install -y --no-install-recommends git doxygen intltool libtool libgtk-3-dev python-lxml python-docutils rst2pdf build-essential git clone --depth=1 https://github.com/geany/geany /geany-test && cd /geany-test PYTHON=python2 ./autogen.sh --enable-api-docs --enable-gtkdoc-header make -C doc cp doc/geany-gtkdoc.h /shared/geany-gtkdoc-py2.h ```
The resulting files are identical. Yay.
Yay, good thought.
@kugel- approved this pull request.
So if you squash and add a little bit more information to the commit message (even if it's just the statement "We migrating away from python2") I'm fine
@elextr do we want to use "python3" in the shebang of the script? ``` diff --git a/scripts/gen-api-gtkdoc.py b/scripts/gen-api-gtkdoc.py index 75beb86c..7423257c 100755 --- a/scripts/gen-api-gtkdoc.py +++ b/scripts/gen-api-gtkdoc.py @@ -1,4 +1,4 @@ -#!/usr/bin/env python +#!/usr/bin/env python3 # # Copyright 2015 The Geany contributors # ```
Technically it is not necessary as the script is called by Autotools with the detected Python interpreter but it would document that the script is meant to be run with Python3.
python3 should always be present, python may not.
python3 should always be present, python may not.
True but as said, in this case it doesn't matter. Autotools detect the Python interpreter to use and then uses this to start the script, i.e. the shebang is not evaluated at all (it executes something like `/usr/bin/python3 scripts/gen-api-gtkdoc.py`).
I guess we agree on the change anyway.
This is a friendly reminder for @elextr :D.
I guess we are pretty much done with this, I think we should update the shebang as said in https://github.com/geany/geany/pull/2903#issuecomment-940454201 but then ready to merge?
Oh, for some reason I thought you had already merged it.
IRO the shebang, as you said in the comment, its not used in the build, it would be only if someone wanted to run the script manually, but doesn't hurt to update it.
@eht16 pushed 1 commit.
51a0fcf36c04a86d8460793dca5da8b50def0b77 Use Python3 shebang for gen-api-gtkdoc.py
@eht16 pushed 2 commits.
6a0a5a76410c898ee97c661bb5e31af560b4cbeb Apply Debian patch for Python 3 10b14dc88eba220b2c21584263c8d54324fe72f3 Use Python3 shebang for gen-api-gtkdoc.py
I added the shebang change myself and rebased on master to fix merge conflicts.
What's missing here? My only objection was the lack of information around the change, not the change itself.
Just came across a system which has python3 but not python.
@kugel- I am not at my development machine to test this for a while, so if it works for you feel free to commit it.
Merged #2903 into master.
github-comments@lists.geany.org