fix buildinfo inconsistency

[adrianschroeter]

We need to send local _service files together with build description
files to get a valid answer.

Old OBS instances can run into an error state now, since the cpio post
is not supported there. But this is better then the wrong answer.

osc buildinfo did not list buildtime service depencies when call in
local working directory, where a build description file exist.

It did work outside of the directory when no local build description
was send.

[M0ses]

Works for me

[marcus-h]

On 2021-08-06 01:01:55 -0700, Adrian Schröter wrote: We need to send local _service files together with build description files to get a valid answer. Old OBS instances can run into an error state now, since the cpio post is not supported there. But this is better then the wrong answer.

I think that's perfectly fine (osc build would also error out in this case).

diff --git a/osc/commandline.py b/osc/commandline.py index 76eb7e5d..f8ad87a7 100644 --- a/osc/commandline.py +++ b/osc/commandline.py @@ -6038,7 +6038,7 @@ def do_buildinfo(self, subcmd, opts, *args): wd = os.curdir args = slash_split(args) - project = package = repository = arch = build_descr = None + project = package = repository = arch = build_descr = service_data = None if len(args) <= 3: if not is_package_dir('.'): raise oscerr.WrongArgs('Incorrect number of arguments (Note: \'.\' is no package wc)') @@ -6048,6 +6048,8 @@ def do_buildinfo(self, subcmd, opts, *args): else: project = store_read_project('.') package = store_read_package('.') + if os.path.exists('_service'): + service_data = open('_service', 'rb').read()

Hmm why only in the else branch? Should we also support a _buildenv file? Should we also support something like $> osc buildinfo prj pkg repo arch foo.spec a_service_file.xml or $> osc buildinfo prj pkg repo arch foo.spec --service-file a_service_file.xml (yes... lot's of strange questions:) )

diff --git a/osc/core.py b/osc/core.py index 1ddb9538..0b8913b4 100644 --- a/osc/core.py +++ b/osc/core.py @@ -6200,7 +6200,7 @@ def get_dependson(apiurl, project, repository, arch, packages=None, reverse=None f = http_GET(u) return f.read() -def get_buildinfo(apiurl, prj, package, repository, arch, specfile=None, addlist=None, debug=None): +def get_buildinfo(apiurl, prj, package, repository, arch, specfile=None, addlist=None, debug=None, servicefile=None):

Minor minor: maybe also call it service_data?

query = [] if addlist: for i in addlist: @@ -6210,8 +6210,19 @@ def get_buildinfo(apiurl, prj, package, repository, arch, specfile=None, addlist u = makeurl(apiurl, ['build', prj, repository, arch, package, '_buildinfo'], query=query) + post_body=specfile

Minor: whitespaces

+ if servicefile: + if specfile == None:

Minor: if specfile is None:

+ print('error - application need to send also the build description file when sending a _service file') + sys.exit(1)

Hmm we should raise a ValueError or RuntimeError instead of a print+sys.exit.

+ from .util import cpio

Minor: we could also move the import to the top of the file (so that we can eventually phase out these kind of imports).

…

+ cpio = cpio.CpioWrite() + cpio.add(b'buildinfo.spec', specfile) + cpio.add(b'_service', servicefile) + post_body = cpio.get() + if specfile: - f = http_POST(u, data=specfile) + f = http_POST(u, data=post_body) else: f = http_GET(u) return f.read()

[adrianschroeter]

On Freitag, 6. August 2021, 14:02:43 CEST Marcus Hüwe wrote: On 2021-08-06 01:01:55 -0700, Adrian Schröter wrote: > We need to send local _service files together with build description > files to get a valid answer. > > Old OBS instances can run into an error state now, since the cpio post > is not supported there. But this is better then the wrong answer. > I think that's perfectly fine (osc build would also error out in this case). > diff --git a/osc/commandline.py b/osc/commandline.py > index 76eb7e5d..f8ad87a7 100644 > --- a/osc/commandline.py > +++ b/osc/commandline.py > @@ -6038,7 +6038,7 @@ def do_buildinfo(self, subcmd, opts, *args): > wd = os.curdir > args = slash_split(args) > > - project = package = repository = arch = build_descr = None > + project = package = repository = arch = build_descr = service_data = None > if len(args) <= 3: > if not is_package_dir('.'): > raise oscerr.WrongArgs('Incorrect number of arguments (Note: \'.\' is no package wc)') > @@ -6048,6 +6048,8 @@ def do_buildinfo(self, subcmd, opts, *args): > else: > project = store_read_project('.') > package = store_read_package('.') > + if os.path.exists('_service'): > + service_data = open('_service', 'rb').read() Hmm why only in the else branch?

we can also do when manually selecting the local build description file in the other case.. will adapt.

Should we also support a _buildenv file?

What would be the usecase to send a _buildenv file to get it basically back?

Should we also support something like $> osc buildinfo prj pkg repo arch foo.spec a_service_file.xml or $> osc buildinfo prj pkg repo arch foo.spec --service-file a_service_file.xml

The service file is always called '_service' or it has no effect ... so this looks like unneeded complexity to me.

(yes... lot's of strange questions:) )

only the last one;)

> diff --git a/osc/core.py b/osc/core.py > index 1ddb9538..0b8913b4 100644 > --- a/osc/core.py > +++ b/osc/core.py > @@ -6200,7 +6200,7 @@ def get_dependson(apiurl, project, repository, arch, packages=None, reverse=None > f = http_GET(u) > return f.read() > > -def get_buildinfo(apiurl, prj, package, repository, arch, specfile=None, addlist=None, debug=None): > +def get_buildinfo(apiurl, prj, package, repository, arch, specfile=None, addlist=None, debug=None, servicefile=None): Minor minor: maybe also call it service_data?

We could ... but we should also renamce specfile then .. but this would be incompatible ./

> query = [] > if addlist: > for i in addlist: > @@ -6210,8 +6210,19 @@ def get_buildinfo(apiurl, prj, package, repository, arch, specfile=None, addlist > > u = makeurl(apiurl, ['build', prj, repository, arch, package, '_buildinfo'], query=query) > > + post_body=specfile Minor: whitespaces

done

> + if servicefile: > + if specfile == None: Minor: if specfile is None:

done

> + print('error - application need to send also the build description file when sending a _service file') > + sys.exit(1) Hmm we should raise a ValueError or RuntimeError instead of a print+sys.exit.

okay

> + from .util import cpio Minor: we could also move the import to the top of the file (so that we can eventually phase out these kind of imports).

okay

…

-- Adrian Schroeter ***@***.***> Build Infrastructure Project Manager SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nuernberg, Germany (HRB 247165, AG München), Geschäftsführer: Felix Imendörffer

[marcus-h]

On 2021-08-06 05:47:33 -0700, Adrian Schröter wrote: On Freitag, 6. August 2021, 14:02:43 CEST Marcus Hüwe wrote: > On 2021-08-06 01:01:55 -0700, Adrian Schröter wrote:

<SNIP>

> > @@ -6048,6 +6048,8 @@ def do_buildinfo(self, subcmd, opts, *args): > > else: > > project = store_read_project('.') > > package = store_read_package('.') > > + if os.path.exists('_service'): > > + service_data = open('_service', 'rb').read() > > Hmm why only in the else branch? we can also do when manually selecting the local build description file in the other case..

Actually, I was talking about the else branch of the "if opts.alternative_project" statement.

will adapt.

See below.

> Should we also support a _buildenv file? What would be the usecase to send a _buildenv file to get it basically back?

Just to have a consistent behavior with "osc build": $> cd prj/pkg_with__buildenv $> osc build $> osc buildinfo IMHO, both osc commands should use the same buildinfo.

> Should we also support something like > > $> osc buildinfo prj pkg repo arch foo.spec a_service_file.xml > or > $> osc buildinfo prj pkg repo arch foo.spec --service-file a_service_file.xml > The service file is always called '_service' or it has no effect ... so this looks like unneeded complexity to me.

"Guessing"/silently using the ./_service file in the "all arguments" case could be problematic: $> cd openSUSE:Tools/osc $> osc buildinfo openSUSE:Tools build standard x86_64 ../build/build.spec In this case, we would silently use osc's _service file.

> > diff --git a/osc/core.py b/osc/core.py > > index 1ddb9538..0b8913b4 100644 > > --- a/osc/core.py > > +++ b/osc/core.py > > @@ -6200,7 +6200,7 @@ def get_dependson(apiurl, project, repository, arch, packages=None, reverse=None > > f = http_GET(u) > > return f.read() > > > > -def get_buildinfo(apiurl, prj, package, repository, arch, specfile=None, addlist=None, debug=None): > > +def get_buildinfo(apiurl, prj, package, repository, arch, specfile=None, addlist=None, debug=None, servicefile=None): > > Minor minor: maybe also call it service_data? We could ... but we should also renamce specfile then .. but this would be incompatible ./

Yes, renaming is not an option:)

@@ -6210,8 +6211,18 @@ def get_buildinfo(apiurl, prj, package, repository, arch, specfile=None, addlist u = makeurl(apiurl, ['build', prj, repository, arch, package, '_buildinfo'], query=query) + post_body = specfile + if servicefile: + if specfile is None: + e.osc_msg = 'error - application need to send also the build description file when sending a _service file' + raise

Hmm this won't work. Just something like raise RuntimeError('application need to...') should do the trick.

+ cpio = cpio.CpioWrite() + cpio.add(b'buildinfo.spec', specfile) + cpio.add(b'_service', servicefile) + post_body = cpio.get() + if specfile:

Minor: "if post_body:" (or "if post_body is not None:") (but feel free to keep it as is:) )

…

- f = http_POST(u, data=specfile) + f = http_POST(u, data=post_body)

[dmach]

Turning into a draft, since there's still an ongoing discussion.

[dmach]

This pull request has been in a draft state for quite some time.
Could you either finish it, close it or add a comment that you're still planning to work on it?