[opensuse-buildservice] osc submitreq accept broken
Hi, see submitreq 315. accepting that one caused KDE:KDE4:Factory:Desktop to fail with "patch 'project.diff' does not exist". what has happened seems to be that the submitreq was accepted, and the _link file got the project.diff's that were in :UNSTABLE:Desktop before, and the project.diff got deleted from the _link file in :UNSTABLE (and removed from the source checkout), but never added to the source checkout of the package in KDE:KDE4:Factory:Desktop. This seems to be a quite recent regression, it used to work fine a couple of days ago still. Greetings, Dirk --------------------------------------------------------------------- To unsubscribe, e-mail: opensuse-buildservice+unsubscribe@opensuse.org For additional commands, e-mail: opensuse-buildservice+help@opensuse.org
On Monday 14 July 2008, Dirk Mueller wrote:
This seems to be a quite recent regression, it used to work fine a couple of days ago still.
hmm, even worse, it seems the src repository is now corrupted. from a fresh checkout: $ osc up -e Expanding to rev e1c5738a807f378e638285c9980c51b2 Server returned an error: HTTP Error 404: Not Found kde4-filesystem/e1c5738a807f378e638285c9980c51b2-kde4-filesystem: not in repository Greetings, Dirk --------------------------------------------------------------------- To unsubscribe, e-mail: opensuse-buildservice+unsubscribe@opensuse.org For additional commands, e-mail: opensuse-buildservice+help@opensuse.org
On Monday 14 July 2008, Dirk Mueller wrote:
hmm, even worse, it seems the src repository is now corrupted. from a fresh checkout:
there are two more bugs in isascii() which might have caused that: - it read 0 bytes of the file at offset 4096 instead of 4096 bytes from offset 0 - a file containing a newline \r or \n was considered to be binary. anyway, I think it might be better to just use the perl builtin operator -T for it (and all the other possible bugs still lurking in 5 lines of perl). @@ -1650,12 +1650,7 @@ sub sourcediff { sub isascii { my ($file) = @_; - local *F; - open(F, '<', $file) || die("$file: $!\n"); - my $buf = ''; - sysread(F, $buf, 0, 4096); - close F; - return 1 unless $buf =~ /[\000-\037]/s; + return 1 if -T $file; return 0; } Greetings, Dirk --------------------------------------------------------------------- To unsubscribe, e-mail: opensuse-buildservice+unsubscribe@opensuse.org For additional commands, e-mail: opensuse-buildservice+help@opensuse.org
On Mon, Jul 14, 2008 at 11:33:23PM +0200, Dirk Mueller wrote:
On Monday 14 July 2008, Dirk Mueller wrote:
hmm, even worse, it seems the src repository is now corrupted. from a fresh checkout:
there are two more bugs in isascii() which might have caused that:
- it read 0 bytes of the file at offset 4096 instead of 4096 bytes from offset 0
- a file containing a newline \r or \n was considered to be binary.
anyway, I think it might be better to just use the perl builtin operator -T for it (and all the other possible bugs still lurking in 5 lines of perl).
@@ -1650,12 +1650,7 @@ sub sourcediff {
sub isascii { my ($file) = @_;
- local *F;
- open(F, '<', $file) || die("$file: $!\n");
- my $buf = '';
- sysread(F, $buf, 0, 4096);
- close F;
- return 1 unless $buf =~ /[\000-\037]/s;
- return 1 if -T $file; return 0;
I don't like that because it's locale dependant. M. --------------------------------------------------------------------- To unsubscribe, e-mail: opensuse-buildservice+unsubscribe@opensuse.org For additional commands, e-mail: opensuse-buildservice+help@opensuse.org
On 2008-07-15 12:18:21 +0200, Michael Schroeder wrote:
On Mon, Jul 14, 2008 at 11:33:23PM +0200, Dirk Mueller wrote:
On Monday 14 July 2008, Dirk Mueller wrote:
hmm, even worse, it seems the src repository is now corrupted. from a fresh checkout:
there are two more bugs in isascii() which might have caused that:
- it read 0 bytes of the file at offset 4096 instead of 4096 bytes from offset 0
- a file containing a newline \r or \n was considered to be binary.
anyway, I think it might be better to just use the perl builtin operator -T for it (and all the other possible bugs still lurking in 5 lines of perl).
@@ -1650,12 +1650,7 @@ sub sourcediff {
sub isascii { my ($file) = @_;
- local *F;
- open(F, '<', $file) || die("$file: $!\n");
- my $buf = '';
- sysread(F, $buf, 0, 4096);
- close F;
- return 1 unless $buf =~ /[\000-\037]/s;
- return 1 if -T $file; return 0;
I don't like that because it's locale dependant.
how about setting the correct locale on startup and still use the perl internals? and can we get rid of the die() error handling there? just because one submitreq is broken we dont need to kill the whole process. darix -- openSUSE - SUSE Linux is my linux openSUSE is good for you www.opensuse.org --------------------------------------------------------------------- To unsubscribe, e-mail: opensuse-buildservice+unsubscribe@opensuse.org For additional commands, e-mail: opensuse-buildservice+help@opensuse.org
On Tue, Jul 15, 2008 at 01:14:45PM +0200, Marcus Rueckert wrote:
how about setting the correct locale on startup and still use the perl internals?
The locale the build service runs in has nothing to do with the locale used in the source files.
and can we get rid of the die() error handling there? just because one submitreq is broken we dont need to kill the whole process.
What die() call? M. --------------------------------------------------------------------- To unsubscribe, e-mail: opensuse-buildservice+unsubscribe@opensuse.org For additional commands, e-mail: opensuse-buildservice+help@opensuse.org
On 2008-07-15 14:42:56 +0200, Michael Schroeder wrote:
On Tue, Jul 15, 2008 at 01:14:45PM +0200, Marcus Rueckert wrote:
how about setting the correct locale on startup and still use the perl internals?
The locale the build service runs in has nothing to do with the locale used in the source files.
and can we get rid of the die() error handling there? just because one submitreq is broken we dont need to kill the whole process.
What die() call?
quoting from dirk's diff: - open(F, '<', $file) || die("$file: $!\n"); -- openSUSE - SUSE Linux is my linux openSUSE is good for you www.opensuse.org --------------------------------------------------------------------- To unsubscribe, e-mail: opensuse-buildservice+unsubscribe@opensuse.org For additional commands, e-mail: opensuse-buildservice+help@opensuse.org
On Tue, Jul 15, 2008 at 03:41:25PM +0200, Marcus Rueckert wrote:
quoting from dirk's diff:
- open(F, '<', $file) || die("$file: $!\n");
Oh, but it can't die here unless the filesystem is corrupt. And in that case, it really should die instead of doing more damage... M. --------------------------------------------------------------------- To unsubscribe, e-mail: opensuse-buildservice+unsubscribe@opensuse.org For additional commands, e-mail: opensuse-buildservice+help@opensuse.org
On 2008-07-15 15:44:44 +0200, Michael Schroeder wrote:
On Tue, Jul 15, 2008 at 03:41:25PM +0200, Marcus Rueckert wrote:
quoting from dirk's diff:
- open(F, '<', $file) || die("$file: $!\n");
Oh, but it can't die here unless the filesystem is corrupt. And in that case, it really should die instead of doing more damage...
there are more graceful ways of handling error conditions. like jumping out of the handling and returning proper errors. darix -- openSUSE - SUSE Linux is my linux openSUSE is good for you www.opensuse.org --------------------------------------------------------------------- To unsubscribe, e-mail: opensuse-buildservice+unsubscribe@opensuse.org For additional commands, e-mail: opensuse-buildservice+help@opensuse.org
On Tue, Jul 15, 2008 at 04:06:09PM +0200, Marcus Rueckert wrote:
On 2008-07-15 15:44:44 +0200, Michael Schroeder wrote:
On Tue, Jul 15, 2008 at 03:41:25PM +0200, Marcus Rueckert wrote:
quoting from dirk's diff:
- open(F, '<', $file) || die("$file: $!\n");
Oh, but it can't die here unless the filesystem is corrupt. And in that case, it really should die instead of doing more damage...
there are more graceful ways of handling error conditions. like jumping out of the handling and returning proper errors.
Sorry, but throwing an exception is exactly the right way. M. --------------------------------------------------------------------- To unsubscribe, e-mail: opensuse-buildservice+unsubscribe@opensuse.org For additional commands, e-mail: opensuse-buildservice+help@opensuse.org
On Wednesday 16 July 2008 11:56:06 Michael Schroeder wrote:
On Tue, Jul 15, 2008 at 04:06:09PM +0200, Marcus Rueckert wrote:
On 2008-07-15 15:44:44 +0200, Michael Schroeder wrote:
On Tue, Jul 15, 2008 at 03:41:25PM +0200, Marcus Rueckert wrote:
quoting from dirk's diff:
- open(F, '<', $file) || die("$file: $!\n");
Oh, but it can't die here unless the filesystem is corrupt. And in that case, it really should die instead of doing more damage...
there are more graceful ways of handling error conditions. like jumping out of the handling and returning proper errors.
Sorry, but throwing an exception is exactly the right way.
It should be said that this die() only affects the running fork for this request, but not kill the entire source server. (I think this caused some misunderstanding here) bye adrian -- Adrian Schroeter SUSE LINUX Products GmbH, GF: Markus Rex, HRB 16746 (AG Nürnberg) email: adrian@suse.de --------------------------------------------------------------------- To unsubscribe, e-mail: opensuse-buildservice+unsubscribe@opensuse.org For additional commands, e-mail: opensuse-buildservice+help@opensuse.org
Am Dienstag, 15. Juli 2008 12:18:21 schrieb Michael Schroeder:
On Mon, Jul 14, 2008 at 11:33:23PM +0200, Dirk Mueller wrote:
On Monday 14 July 2008, Dirk Mueller wrote:
hmm, even worse, it seems the src repository is now corrupted. from a fresh checkout:
there are two more bugs in isascii() which might have caused that:
- it read 0 bytes of the file at offset 4096 instead of 4096 bytes from offset 0 - a file containing a newline \r or \n was considered to be binary.
anyway, I think it might be better to just use the perl builtin operator -T for it (and all the other possible bugs still lurking in 5 lines of perl). Sorry, but this is the kind of code which makes it ugly. It takes one compareable long to understand the code - not speaking about nice error possibilities.
@@ -1650,12 +1650,7 @@ sub sourcediff {
sub isascii { my ($file) = @_;
- local *F;
- open(F, '<', $file) || die("$file: $!\n");
- my $buf = '';
- sysread(F, $buf, 0, 4096);
- close F;
- return 1 unless $buf =~ /[\000-\037]/s;
- return 1 if -T $file; According to manpage: return 1 if( -f $file && -T $file );
I don't like that because it's locale dependant. From the manpage:
| The "-T" and "-B" switches work as follows. The first block or so of the | file is examined for odd characters such as strange control codes or | characters with the high bit set. If too many strange characters (>30%) | are found, it's a "-B" file; otherwise it's a "-T" file. Also, any | file containing null in the first block is considered a binary file. Klaas -- Klaas Freitag Architect OPS/IPD SUSE LINUX Products GmbH - Nuernberg --------------------------------------------------------------------- To unsubscribe, e-mail: opensuse-buildservice+unsubscribe@opensuse.org For additional commands, e-mail: opensuse-buildservice+help@opensuse.org
On Tuesday 15 July 2008, Klaas Freitag wrote:
anyway, I think it might be better to just use the perl builtin operator -T for it (and all the other possible bugs still lurking in 5 lines of perl). Sorry, but this is the kind of code which makes it ugly.
Using -T makes it ugly or not using -T makes it ugly? If the first: thats why I didn't remove the isascii() function. the implementation of isascii() should be irrelevant as long as you, when you see a usage of the function in the code somewhere else know what it does. if you don't know, a comment at the function header, that describes assumptions, result and purpose of a function helps a lot more than having to first understand the purpose of a function by reading it. It is for example not immediately clear that the isascii() function actually checks for "not binary" and not for ASCII-ness. A latin1 encoded file would true for isascii() even though it is anything but ascii. None of those things were changed or made worse by my commit. I also disagree that replacing code that was obviously never tested (I've fixed 3 bugs in the total 6 lines of perl!) with a builtin function is inherently wrong or making things more ugly. Instead it wrapped an ununderstandable perl builtin function detail in a descriptive function name. (which I think is not descriptive, but I don't own the code, I just want it to work :) ). Using builtins is probably also faster than a openly coded regular expression over a long binary string, but performance shouldn't be a concern here. Third: lets not start coding style discussions when it comes to OBS backend, mkay? functions that are more than 200+ lines long, that are not modularized, commented, are using quite a big set of perl tricks everywhere, use and update global variables at random places while expecting them to never change (due to longrunning iterators over them), do not have clear error recovery paths and not even testcases is not somethen were we should start discussing about a 6 line helper function IMHO. Anyway: I accept that the undefinedness of -T's exact way of working is probably making it a bad candidate for usage here, even though I would prefer to use it for the sole reason of being engineered, tested and maintained elsewhere. Greetings, Dirk --------------------------------------------------------------------- To unsubscribe, e-mail: opensuse-buildservice+unsubscribe@opensuse.org For additional commands, e-mail: opensuse-buildservice+help@opensuse.org
On Mon, Jul 14, 2008 at 08:06:35PM +0200, Dirk Mueller wrote:
On Monday 14 July 2008, Dirk Mueller wrote:
This seems to be a quite recent regression, it used to work fine a couple of days ago still.
hmm, even worse, it seems the src repository is now corrupted. from a fresh checkout:
This was already fixed some days ago. Check the putinsrcrep() call in integratelink(), there should be a link() call right before putinsrcrep. M. --------------------------------------------------------------------- To unsubscribe, e-mail: opensuse-buildservice+unsubscribe@opensuse.org For additional commands, e-mail: opensuse-buildservice+help@opensuse.org
participants (5)
-
Adrian Schröter -
Dirk Mueller -
Klaas Freitag -
Marcus Rueckert -
Michael Schroeder