Opened 4 years ago

Last modified 3 years ago

#2872 assigned defect

No error is reported when suppressionFile does not exist

Reported by: wentasah Owned by: MikeLing
Priority: minor Milestone: 0.8.x
Version: Keywords: simple
Cc:

Description

When a WarningCountingShellCommand? build step is created with suppressionFile keyword and the file referred by it cannot be accessed, this is not reported as an error. Instead, the step is run as if the suppressionFile is empty.

It seems that steps/shell.py tries to handle errors in uploadFile command, but this doesn't work.

Attachments (1)

0001-Show-error-when-suppression-File-does-not-exist.patch (2.2 KB) - added by MikeLing 3 years ago.
PR for issue 2872

Download all attachments as: .zip

Change History (14)

comment:1 Changed 4 years ago by dustin

  • Keywords simple added
  • Milestone changed from undecided to 0.9.0

comment:2 Changed 4 years ago by dustin

  • Version 0.8.9 deleted

comment:3 Changed 4 years ago by dustin

  • Milestone changed from 0.9.0 to 0.8.x

comment:4 Changed 3 years ago by MikeLing

  • Owner set to MikeLing
  • Status changed from new to assigned

comment:5 Changed 3 years ago by MikeLing

Hello, I got some progress after I assign this to myself.

Fist of all, I suppose we need add 'isfile' check around this line (https://github.com/buildbot/buildbot/blob/master/slave/buildslave/commands/transfer.py#L85) and return with a 'file doesn't exist' error.

Then we need add a test for it in https://github.com/buildbot/buildbot/blob/6b883c118c37709835bfb37e69af4990796843fe/master/buildbot/test/regressions/test_steps_shell_WarningCountingShellCommand.py. Is that right?

But my question is how to init WarningCountingShellCommand? if I want to add suppression with suppressionFile. Something like " w = WarningCountingShellCommand? suppressionFile='impossiblefilename.txt')" doesn't work because it told me:

exceptions.AttributeError?: 'NoneType?' object has no attribute 'getSlaveCommandVersion'

Could you tell me what should I do next? Thank you very much! :)

comment:6 Changed 3 years ago by dustin

Calling

WarningCountingShellCommand(suppressionFile='some-filename.txt')

should work fine without that error (which comes later when the step runs). In fact, that's basically what the existing test does.

Changed 3 years ago by MikeLing

PR for issue 2872

comment:7 Changed 3 years ago by MikeLing

How could I test the values of stderr and rc which in https://github.com/buildbot/buildbot/blob/6b883c118c37709835bfb37e69af4990796843fe/slave/buildslave/commands/transfer.py#L96-L97 for testing WarningCountingShellCommand?.

BTW, could I create a PR and ask question on there? Anyway, I had add a patch file for it :)

comment:8 follow-up: Changed 3 years ago by dustin

In your attachment, you've written the first few lines of a test for this issue. And your fix in transfer.py looks good, too. It appears you've found the root of the bug, which is that SlaveFileUploadCommand does not correctly return an error when the file does not exist.

Let's try to solve that first, by updating the unit test under slave/buildslave/test to correctly show the problem, and then applying your fix to show that it actually solves the problem.

Buildbot's unit tests only run master code or slave code, not both -- so if this is the only problem (and I suspect it is), then you can only test it in the slave tests.

comment:9 in reply to: ↑ 8 Changed 3 years ago by MikeLing

Replying to dustin:

Let's try to solve that first, by updating the unit test under slave/buildslave/test to correctly show the problem, and then applying your fix to show that it actually solves the problem.

Yeah, but after read those tests in there, I found we already have the test with doesn't exist file(https://github.com/buildbot/buildbot/blob/master/slave/buildslave/test/unit/test_commands_transfer.py#L176).

And after a second thought about it, I suddenly notice the 'open'(https://github.com/buildbot/buildbot/blob/1147bf7a0ebe472a63ad5f7eef473648bb805a14/slave/buildslave/commands/transfer.py#L91) itself should return 'No such file or directory:' error with an unexist file path, and it should been catch as an 'Exception' I think. Therefore, my fix in transfer.py is redundant code for it. :)

So I'm wondering if we still need work on this issue? Maybe it had been fixed accidently? ;)

Please tell me if I misunderstand something or went into wrong direction, thank you!

comment:10 follow-up: Changed 3 years ago by dustin

You might be right -- the way to find out is to set up a test master and configure it with the situation that should fail, and see what happens.

comment:11 in reply to: ↑ 10 Changed 3 years ago by MikeLing

Replying to dustin:

You might be right -- the way to find out is to set up a test master and configure it with the situation that should fail, and see what happens.

Sure! Could you give me some hints about how exactly reproduce it? Thank you :)

comment:12 follow-up: Changed 3 years ago by dustin

Once you have a working master (basically, by following the tutorial), add a WarningCountingShellCommand with a suppressionFile that does not exist, force a build, and see what happens :)

comment:13 in reply to: ↑ 12 Changed 3 years ago by MikeLing

Replying to dustin:

Once you have a working master (basically, by following the tutorial), add a WarningCountingShellCommand with a suppressionFile that does not exist, force a build, and see what happens :)

Sorry for the late reply! I just busy with some other works and the my new semester just start. Sorry

you're right, we still need to work on this issue(I mean need some more work to fix this bug)

I set up the buildbot config like:

factory.addStep(steps.Compile(command=make?,

warningPattern="(.\*?):([0-9]+): [Ww]arning: (.\*)$", warningExtractor=steps.Compile.warnExtractFromRegexpGroups, suppressionFile="support-files/noexistfile.supp"))

and I got failure and output like this https://pastebin.mozilla.org/8863315 But I can't find any log message like "Cannot open file noexistfile.supp for upload" and don't know what exactly suppressionFile been uploaded(not exist file or just a None)

I will look more into this :)

Note: See TracTickets for help on using tickets.