xpra icon
Bug tracker and wiki

Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#332 closed defect (fixed)

0.9.1: no "About Xpra" dialog

Reported by: onlyjob Owned by: onlyjob
Priority: major Milestone: 0.10
Component: core Version:
Keywords: Cc:

Description

0.9.1 no longer able to show "About Xpra" dialog:

Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/xpra/scripts/about.py", line 78, in about
    dialog.set_comments("\n".join(get_build_info()))
  File "/usr/lib/python2.7/dist-packages/xpra/scripts/config.py", line 190, in get_build_info
    warn("Error: could not find the build information: %s", e)
TypeError: warn() takes exactly 1 argument (2 given)

This is a regression from 0.9.0.

Change History (24)

comment:1 Changed 7 years ago by Antoine Martin

Component: androidcore
Owner: changed from Antoine Martin to onlyjob

That's odd, the build_info.py file is created as part of the build.
Do you have any changes, maybe as part of #331, that takes it out somehow?

Are you sure that this is a regression from 0.9.0?? (nothing has changed in this area between 0.9.0 and 0.9.1). If so, please bisect it (only 5 commits - should take 3 steps at most, more likely just 2) - as I am unable to reproduce this here.

r3360 avoids the double error on the error path and should allow you to see the dialog, but it does not explain why your build is missing the build_info.py - that should get fixed too.

comment:2 Changed 7 years ago by onlyjob

Ironically my build is not missing "build_info.py": here is what's in it:

LOCAL_MODIFICATIONS='unknown'
BUILD_DATE='2013-05-11'
BUILT_BY='pbuilder'
BUILT_ON='deblab'
BUILD_BIT='64bit'
BUILD_CPU='Intel(R) Pentium(R) D CPU 3.00GHz'
RELEASE_BUILD='True'
REVISION='unknown'

I was suspecting that it could it be effect of backporting r3353 but file got correctly re-generated on build...

I'll try to bisect when I can. At least I can tell that "build_info.py" is generated and installed to package...

comment:3 Changed 7 years ago by Antoine Martin

That build_info.py must be missing from your installation, otherwise you would not be seeing this error.

BTW, I'm not convinced you should be backporting stuff like r3353 when the packages have been built like this for a very long time and the next minor update is just a few days away...

comment:4 Changed 7 years ago by onlyjob

It's not missing: it is in file system and in the package.

The reason for backporting is that I can't build the package in "pbuilder" without "cython" outside of chroot... It fails on "clean" even before attempting to initialise chroot, as I explained in #331. I'm trying to backport only what's necessary, and I don't even have spare time to backport what I don't have to, least to say that I might be too lazy to do that. :)

As for next minor release it's great but since I can't read your mind I never know how soon next release is going to be...

comment:5 Changed 7 years ago by Antoine Martin

If the file is there, this should work:

python -c "from xpra.scripts.config import get_build_info;print(get_build_info())"

But I suspect it will not.
It is pretty much equivalent to this (which should also work):

python -c 'from xpra.build_info import BUILT_BY, BUILT_ON, BUILD_DATE, REVISION, LOCAL_MODIFICATIONS;
print(", ".join([BUILT_BY, BUILT_ON, BUILD_DATE, REVISION, LOCAL_MODIFICATIONS]))'

As for backports.. I did say:

I will backport to 0.9.x

And minor releases do happen fairly regularly, when needed.

comment:6 Changed 7 years ago by onlyjob

For what's it worth:

On Xpra-0.9.0 strace python2.7 -c "from xpra.scripts.config import get_build_info;print(get_build_info())" 2>&1 | grep build_

stat("/usr/lib/python2.7/dist-packages/xpra/build_info", 0x7fffa9e81820) = -1 ENOENT (No such file or directory)
open("/usr/lib/python2.7/dist-packages/xpra/build_info.x86_64-linux-gnu.so", O_RDONLY) = -1 ENOENT (No such file or directory)
open("/usr/lib/python2.7/dist-packages/xpra/build_info.so", O_RDONLY) = -1 ENOENT (No such file or directory)
open("/usr/lib/python2.7/dist-packages/xpra/build_infomodule.so", O_RDONLY) = -1 ENOENT (No such file or directory)
open("/usr/lib/python2.7/dist-packages/xpra/build_info.py", O_RDONLY) = 3
open("/usr/lib/python2.7/dist-packages/xpra/build_info.pyc", O_RDONLY) = 4

On Xpra-0.9.1 strace python2.7 -c "from xpra.scripts.config import get_build_info;print(get_build_info())" 2>&1 | grep build_:

stat("xpra/build_info", 0x7fff155f6840) = -1 ENOENT (No such file or directory)
open("xpra/build_info.x86_64-linux-gnu.so", O_RDONLY) = -1 ENOENT (No such file or directory)
open("xpra/build_info.so", O_RDONLY)    = -1 ENOENT (No such file or directory)
open("xpra/build_infomodule.so", O_RDONLY) = -1 ENOENT (No such file or directory)
open("xpra/build_info.py", O_RDONLY)    = -1 ENOENT (No such file or directory)
open("xpra/build_info.pyc", O_RDONLY)   = -1 ENOENT (No such file or directory)
write(2, "  File \"xpra/scripts/config.py\","..., 61  File "xpra/scripts/config.py", line 190, in get_build_info

comment:7 Changed 7 years ago by Antoine Martin

You're running it from the source tree I think ("."), which doesn't have it since it's built into somewhere else via your build scripts.

It should be fine once deployed though, unless your current working directory also has an "xpra/init.py" (then it's down to the python interpreter where it loads things from - not us)

And btw, I've reviewed every single changeset for 0.9.1 and none of them have anything to do with paths or anything like that, so I am 99% confident the problem is at your end with your (new?) build scripts.

comment:8 Changed 7 years ago by onlyjob

I _never_ run Xpra from source tree. I don't even know how. :)

I always build packages so I can see the difference between them.
CWD was "/" for running those commands....

I also suspect that it could be build-related but I have only 3 commits between 0.9.0 and 0.9.1 and I can't quite see where it got broken... I'll try to rebuild 0.9.0 again to see if it still works in case something has changed in toolchain...

comment:9 Changed 7 years ago by onlyjob

Xpra-0.9.0:

python2.7 -c "from xpra.scripts.config import get_build_info;print(get_build_info())"

('Error: could not find the build information: %s', ValueError("invalid literal for int() with base 10: 'unknown'",))
['Built on deblab by debdev', '2013-05-12']

But due to "warn" modification in xpra/scripts/config.py the above warning stops: TypeError: warn() takes exactly 1 argument (2 given).

The fix for Xpra-0.9.1 is trivial:

--- a/xpra/scripts/config.py
+++ b/xpra/scripts/config.py
@@ -186,9 +186,9 @@
             info.append("revision %s" % REVISION)
         else:
             info.append("revision %s with %s local changes" % (REVISION, LOCAL_MODIFICATIONS))
     except Exception, e:
-        warn("Error: could not find the build information: %s", e)
+        warn("Warning: could not find the build information: %s" % e)
     return info
 
 
 def read_config(conf_file):

I also changed "Error" --> "Warning" as the latter IMHO is more adequate because it should be OK to continue on such minor problem.

This patch is tested.

comment:10 Changed 7 years ago by Antoine Martin

This is a workaround, not the actual fix: identical to r3360 - see comment:1.
Except that missing components are real errors, not warnings: the problem is with packaging and changing the wording to try to make it sound like something isn't broken is not going to get merged.


The real problem is that the build process loses parts of the build info file. In particular, LOCAL_MODIFICATIONS which ends up being set to "unknown" and then we fail trying to parse it. Why, I'm not sure yet.

  • r3361 fixes a trunk-only bug: it should ensure that we don't clear the build information which is recorded when we generate the source archives ("sdist" command)
  • r3362 makes the parsing code more defensive so that even if an invalid value is present, this won't cause a parsing error
Last edited 7 years ago by Antoine Martin (previous) (diff)

comment:11 Changed 7 years ago by onlyjob

r3360 is a fix. Just applying r3360 is enough to restore "About Xpra" dialog for 0.9.1 just like it was working in 0.9.0.

Please forgive me for misleading information but "build_info.py" was never lost. I think you were right when you suggested that I was invoking command from Xpra source tree. Nevertheless the problem is quite clear from report and first two comments.

The underlying problem is in "add_build_info.py" which tries to create "build_info.py" by extracting metadata from SVN tree. However this is meaningless for tar releases and that's how "build_info.py" from comment2 was created. That's why r3360 fixes the problem with dialog but of course it still complains about "build information".

With next official Debian package (that is based on your tar releases) build information will be populated using the following patch to get rid of warning and to provide correct revision:

--- a/add_build_info.py
+++ b/add_build_info.py
@@ -36,9 +36,9 @@
                 "REVISION" : "unknown",
                 "LOCAL_MODIFICATIONS" : "unknown"
             }
     #find revision:
-    proc = subprocess.Popen("svnversion -n", stdin=None, stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=True)
+    proc = subprocess.Popen("cat svn-version", stdin=None, stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=True)
     (out, _) = proc.communicate()
     if proc.returncode!=0:
         print("'svnversion -n' failed with return code %s" % proc.returncode)
         return  props
@@ -63,9 +63,9 @@
     rev = int(rev_str)
     props["REVISION"] = rev
     #find number of local files modified:
     changes = 0
-    proc = subprocess.Popen("svn status", stdin=None, stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=True)
+    proc = subprocess.Popen("echo 0", stdin=None, stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=True)
     (out, _) = proc.communicate()
     if proc.poll()!=0:
         print("could not get status of local files")
         return  props

comment:12 Changed 7 years ago by Antoine Martin

It is not a fix, it is a kludge. I know it restores "about xpra" but it certainly does not fix the underlying broken packaging.

The change in comment:11 is just plain wrong: only hacking away at a problem rather than figuring out where the file does get lost. (it does get deleted/overwritten/lost somewhere - and this is a debian problem only, rpms are ok)

add_build_info is not meant to override the contents of xpra/build_info with bad values - ever. It only replaces the existing values when it does manage to invoke svnversion / svn status.

And when building packages from a tree without svn, it will fail - which is absolutely fine: in that case it should leave the existing values which come in the source archive and only append what it has access to (build host, etc):

$ cat xpra-0.9.1/xpra/build_info.py 
LOCAL_MODIFICATIONS='0'
REVISION='3332'
RELEASE_BUILD='True'

The problem here is that the file is missing, so the bad values get saved as defaults.

comment:13 in reply to:  12 Changed 7 years ago by onlyjob

Replying to totaam:

The problem here is that the file is missing, so the bad values get saved as defaults.

We can define problem like this but to me main and the most user-visible problem was with incorrect handling of warning for "bad values".

From packaging prospective build_info.py either should be re-generated from scratch or do not change. It is not particularly important whenever this file is not getting deleted during "clean" or if it is correctly generated outside of SVN tree on build-time... But packaging will be unnecessary complicated if build_info.py or other files are modified in place during build so please let's not do that...

comment:14 Changed 7 years ago by Antoine Martin

We can define problem like this..

I am not defining anything, this is what happens and is the real source of the problems. That's a fact.

But packaging will be unnecessary complicated if build_info.py or other files are modified in place during build so please let's not do that...

This is how it is, and has been for a while.
Packaging is a curse and I wished it did not exist. But it does.
Now, until someone re-writes the code and makes it work on all the platforms with all the different packaging solutions, this is how it will remain.

comment:15 in reply to:  14 Changed 7 years ago by onlyjob

Replying to totaam:

I am not defining anything, this is what happens and is the real source of the problems. That's a fact.


Whatever. :) At least we've identified yet another problem and nailed it. :)

From my prospective we can close this bug if you can accommodate r3360 to next 0.9.2 release...

I think we've learned the lesson to be careful with build_info.py as well and to make sure that it contains valid data.

Now it's all good, besides what you already did no other changes are needed here.

Thanks.

Packaging is a curse and I wished it did not exist. But it does.
Now, until someone re-writes the code and makes it work on all the platforms with all the different packaging solutions, this is how it will remain.


That sound a bit annoyed (or stressed?)... I hope that's not because of me...

comment:16 Changed 7 years ago by Antoine Martin

Whatever. :) At least we've identified yet another problem and nailed it. :)

No, we haven't.
Now I have to figure out how to prevent debuild from squashing/deleting that file.

That sound a bit annoyed (or stressed?)...

Yes, I would rather be doing other things than packaging.

comment:17 in reply to:  16 Changed 7 years ago by onlyjob

Replying to totaam:

Whatever. :) At least we've identified yet another problem and nailed it. :)

No, we haven't.


I meant the problem with broken warning. It was fixed, wasn't it? (certainly it was a minor problem)

Now I have to figure out how to prevent debuild from squashing/deleting that file.


Or can you make sure that it re-created properly with correct data?
Also it would be OK to leave the file as is and do not touch it on build.

You can avoid deletion by removing file from debian/clean.

Yes, I would rather be doing other things than packaging.


Debian packages will be closely following your releases so you don't have to worry about this unless you want to provide your own packages.

comment:18 Changed 7 years ago by Antoine Martin

You can avoid deletion by removing file from debian/clean.

AFAICT this file is not in debian/clean

Debian packages will be closely following your releases so you don't have to worry about this unless you want to provide your own packages.

Well, Debian Wheezy ships a buggy version, and let's not even talk about Squeeze, or all the Ubuntus.

So yes, I will continue to provide packages so people can get a decent version of xpra without too much effort. As I have done for 4 years now.

Last edited 7 years ago by Antoine Martin (previous) (diff)

comment:19 in reply to:  18 Changed 7 years ago by onlyjob

Replying to totaam:

You can avoid deletion by removing file from debian/clean.

AFAICT this file is not in debian/clean


Then it is not a packaging problem but perhaps a python build system conf. :)

Well, Debian Wheezy ships a buggy version


Please speak to us about such issues. Until it was released you never mentioned that you don't want it to be in Wheezy. (I vaguely remember that you've said something that 0.3.* is made for distributions).
We could exclude Xpra from stable Debian release and provide an up-to-date version through backports. By the way I'm going to keep fresh Xpra version in backports anyway...

, and let's not even talk about Squeeze, or all the Ubuntus.


I'm with you, but once again please let us know if you want something excluded and we'll see what can be done.


So yes, I will continue to provide packages so people can get a decent version of xpra without too much effort. As I have done for 4 years now.


:)

comment:20 Changed 7 years ago by Antoine Martin

I vaguely remember that you've said something that 0.3.* is made for distributions

Yes, it definitely was, and then things moved on to the newer stable branches and this one was left to rot.
I was spending a lot of effort maintaining multiple stable branches so that distributions could update at their own pace and avoid the problems with bleeding edge versions, but when I realized that they were just sticking with the old buggy one I gave up on them.

No-one should be running 0.3.x right now, the fact that debian "stable" ships this very buggy version speaks for itself..

Last edited 7 years ago by Antoine Martin (previous) (diff)

comment:21 in reply to:  20 Changed 7 years ago by onlyjob

Replying to totaam:

No-one should be running 0.3.x right now, the fact that debian "stable" ships this very buggy version speaks for itself..


Well that warning came too late... Why you didn't tell us not to ship it? We could remove it but you never warned us...

Maybe even a note or warning in README could do... Nevertheless I'm sure we'll communicate better in the future. :)

comment:22 Changed 7 years ago by Antoine Martin

Why you didn't tell us...

Oh, I did.

comment:23 Changed 7 years ago by Antoine Martin

Resolution: fixed
Status: newclosed

r3781 solves this properly for 0.10 by splitting the build info (generated at build time) from the source info

comment:24 Changed 7 years ago by Antoine Martin

Milestone: 0.10

added milestone

Note: See TracTickets for help on using tickets.