search

softwarefactory-project / rdopkg

power to the packagers
Apache License 2.0
28 stars 21 forks source link

DROP-In-RPM specifier and exit status #150

Closed dsariel closed 6 years ago

dsariel commented 6 years ago

Seems that when patch [1] is applied to cinder rhos8 is fails because "No patches changed". While the same patch is applied to rhos9 branch, everything is ok. Attaching full outputs of 'rdopkg patch' for rhos8 and rhos9 with the same patch [2]. Thanks

[1] https://url.corp.redhat.com/patch-2018-Jan-18-rdopkg

[2] https://nofile.io/f/JH9oxrPqj2H/Screenshot+from+2018-01-26+15-03-49.png

yazug commented 6 years ago

can you leave a public link to [1] please?

yazug commented 6 years ago

patches_ignore and patching and patches_base behavior differences. Can you help identify the case that is failing and the case that works?

dsariel commented 6 years ago

Jon, thanks for looking on this. [1] is a link for codeeng, so I'm affrayed I can't.

dsariel commented 6 years ago

Re "patches_ignore and patching and patches_base behavior differences. Can you help identify the case that is failing and the case that works?"

Yeah. Created an internal Jira ticket with more info and added you as a watcher.

ssbarnea commented 6 years ago

Can we have a confirmation of this bug and if this is expected behaviour or if this is expected to be fixed?

yac commented 6 years ago

Sorry for late response, I was sick.

I don't quite see any problem here.

distgit in question is openstack-cinder. Both rhos-8.0-rhel-7 and rhos-9.0-rhel-7 contain

# patches_ignore=DROP-IN-RPM

in .spec and the patch in question has "(DROP-IN-RPM)" in subject so you explicitly told rdopkg not to include that patch in .spec file.

On rhos-8.0-rhel-7, with the patch cherry-picked in local rhos-8.0-patches, rdopkg patch -l results in noop because there are no new patches (you filtered the new one out, output says "7 filtered out by regex" instead of previous "6" so all is as it should be).

On rhos-9.0-rhel7, with the patch cherry-picked in local rhos-9.0-patches, rdopkg patch -l results in a new "Updated patches" commit because the patches branch changed from the last time they were generated (probably a rebase), but the new patch is again filtered out as it should (rdopkg says "3 filtered out by regex" instead of "2" without the patch as expected).

Either way, I see no bug here.

ssbarnea commented 6 years ago

If I am not mistaken, what we want here to obtain is the ability to run rdopkg on a an empty patch and get a non-error error code. This is important for us in order to validate the build process (including finding out if the .spec is ok, if CR comment is ok, ....

My original intent was to look inside the output of rdopkg and check for No patches changed message and treat this as a success instead of failure. Still, others argued that this would be risky because this message could also appear when not expected, including real failures that we want to spot.

I do understand that this verification may be a little bit outside the original scope of rdopkg and that in this case we may want to enable it with an optional parameter instead of having to introspect the stdout/stderr output in order to make a decision if it was a genuine failure or not. What do you think about this?

yac commented 6 years ago

@ssbarnea That is a very reasonable request, I'd be happy to make error codes and messages more useful. Please make your above comment into a separate Issue listing all the requirements and problems and use cases.

I'm even happy to change default meanings of error codes to suit your automated needs.

I'm gonna close this invalid bug and let's continue on a proper one, OK?

dsariel commented 6 years ago

@yac, thanks for taking a look on this. From ordinary user perspective (I mean not at the expert level as you) the current behaviour is confusing since the documentation doesn't require: a) to run 'rdopkg patch -l' and then, b) upon the output, to decide whether to run 'rdopkg patch'

Improving error codes and making messages more useful is of course very welcome (and bit thank you for maintaining this package), but, IMHO, the issue I have encountered is not related to error codes and messages, but rather to the way the rdopkg is used. If 'rdopkg patch' shell be used in 2 steps ( step a and step b as above) it will be useful to have this documented (and I will align with the documantation). Otherwise, it should be no difference in exit code between the rhos8 and rhos9.

dsariel commented 6 years ago

s/bit/big

yac commented 6 years ago

@dsariel You're making assumptions real quick. I didn't imply you should run rdopkg -l patch followed by rdopkg patch in any way. I only ran -l to test locally, it's completely the same as introducing the patch into remote patches branch and running without -l including same exit codes.

Similarly, you assumed that "While the same patch is applied to rhos9 branch, everything is ok." but that wasn't the case, the patch wasn't applied and rdopkg was pretty clear it wasn't applied and git show would tell you the same. rdopkg returned 0 because the patches (already present in distgit) changed in patches branch. If you had clean patches branch, rdopkg would fail with 'No new patches' as it did for rhos-8. I already explained that in detail in previous comment yet you mention it again as if you didn't read it at all.

You run rdopkg patch OR rdopkg -l patch depending on what you want to do and then inspect output. The point is to always run a single command for a single operation. I see no problem with that or how rdopkg is used, I only see problem of people not being able to easily tell whether the run was successfull or not.

That's mainly because running rdopkg patch while there are no new patches returns non-zero exit code which made sense for some interactive and scripting use cases, but you don't seem to care whether there were new patches, only whether the patching process was successful or not if I understand correctly.

Finally, you can edit your posts in-place on github using the pencil icon, no need for s/this/dat/ comments. I wish this was possible in certain other systems as well ;)

yac commented 6 years ago

@dsariel Let me repeat this for a third time just to be sure. You got different result for rhos-8 and rhos-9 because rhos-8 was in sync with its patches branch and rhos-9 was not. That's completely unrelated to the new patch you introduced.