Skip to content

/ saltfs

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add description to buildbot steps #438

Merged
merged 1 commit into from Aug 4, 2016
Merged

Add description to buildbot steps #438

merged 1 commit into from Aug 4, 2016

Conversation

@UK992
Copy link
Contributor

UK992 commented Jul 13, 2016 •
edited by larsbergstrom

Fixes #337

This change is Reviewable
@UK992 UK992 force-pushed the UK992:buildbot-steps branch from f7b570c to dcc80ea Jul 13, 2016
@aneeshusa
Copy link
Member

aneeshusa commented Jul 14, 2016

Looking at the docs again and the current waterfall, I actually think a slightly different approach is better here. The raw ShellCommands (not Compile or Test) already put the whole command as the description, so let's not do anything in that case. Furthermore, what we need to do is set descriptionSuffix for Compile and Test, which will get appended to the description and descriptionDone.

We should be smart about setting descriptionSuffix to avoid noise; here's some good heuristics:

  • No need to include "./mach"
  • Include the mach subcommand, like "test-wpt"
  • Don't include logging info, like --log-raw test-wpt.log
  • Keep anything else, e.g. --release.

An example:

./mach test-wpt --release --processes 24 --log-raw wpt.log --log-errorsummary wpt-errorsummary.log

would turn into a descriptionSuffix of test-wpt --release --processes 24.

Since the descriptionSuffix already includes the operation (e.g. test-wpt implies testing), I think we could just set description to running, and descriptionDone to ran.

Thoughts @edunham ?
@aneeshusa
Copy link
Member

aneeshusa commented Jul 14, 2016

Also, please make the same change to both make_step functions.
@UK992 UK992 force-pushed the UK992:buildbot-steps branch from dcc80ea to 23cc7fc Jul 14, 2016
@UK992
Copy link
Contributor Author

UK992 commented Jul 14, 2016

Done!
buildbot/master/files/config/factories.py Outdated
@@ -110,6 +110,22 @@ def make_step(self, command):
step_kwargs['logEnviron'] = False
step_env += envs.upload_nightly

step_desc_suffix = []

This comment has been minimized.

Sign in to view
@aneeshusa

aneeshusa Jul 18, 2016

Member

I'd prefer to integrate this code into the loop above instead of trying to re-parse things like --log-.*. Here's a rough sketch of one way to do it, feel free to modify as needed for ergonomics:

  • Create a step_desc list before the for arg in args: loop.
  • Inside the ./mach block, append both the ./mach token and the mach_arg token to step_desc.
  • Don't change the log block, so those tokens won't get added to the description.
  • Add an else or similar at the end to append any other tokens to the description. (I think you might have to change the last elif to accommodate this properly.)
buildbot/master/files/config/factories.py
@@ -82,7 +82,7 @@ def make_step(self, command):

command = command.split(' ')

# Add bash -l before every command on Windows builders
# Add `bash -l` before every command on Windows builders

This comment has been minimized.

Sign in to view
@aneeshusa

aneeshusa Jul 18, 2016

Member

Please make the same change in the other make_step function.
@aneeshusa aneeshusa self-assigned this Jul 18, 2016
@UK992 UK992 force-pushed the UK992:buildbot-steps branch from 23cc7fc to 883ab15 Jul 18, 2016
@UK992
Copy link
Contributor Author

UK992 commented Jul 18, 2016

Done.
Now command like:
./mach test-wpt --release --binary-arg=--multiprocess --processes 24 --log-raw test-wpt-mp.log --log-errorsummary wpt-mp-errorsummary.log eventsource

will be turned into: test-wpt --release --binary-arg=--multiprocess --processes 24 eventsource

It is ok to keep eventsource or should be also ignored?
@bors-servo
Copy link
Contributor

bors-servo commented Jul 19, 2016

☔ The latest upstream changes (presumably #441) made this pull request unmergeable. Please resolve the merge conflicts.
@edunham
Copy link
Contributor

edunham commented Jul 19, 2016

Looks good to me; r=me with the merge conflict fixed
@UK992 UK992 force-pushed the UK992:buildbot-steps branch from 883ab15 to 3121d2e Jul 19, 2016
@UK992
Copy link
Contributor Author

UK992 commented Jul 19, 2016

rebased.
@bors-servo
Copy link
Contributor

bors-servo commented Jul 20, 2016

☔ The latest upstream changes (presumably #442) made this pull request unmergeable. Please resolve the merge conflicts.
@UK992 UK992 force-pushed the UK992:buildbot-steps branch from 3121d2e to 44b014e Jul 20, 2016
@aneeshusa
Copy link
Member

aneeshusa commented Jul 20, 2016

r- from me on this, this doesn't look like it works (confirmed with a quick Python 2 REPL), but leaves the description as just one token.

I will follow up with details shortly.
@UK992
Add description to buildbot steps
Loading status checks…
74de967
@UK992 UK992 force-pushed the UK992:buildbot-steps branch from 195c2ac to 74de967 Jul 20, 2016
@UK992
Copy link
Contributor Author

UK992 commented Jul 20, 2016 •
edited

I added missing + in step_desc, if that's what you think.
@jdm
Copy link
Member

jdm commented Jul 27, 2016

This is waiting on feedback from @aneeshusa.
@aneeshusa
Copy link
Member

aneeshusa commented Aug 4, 2016

Sorry about the delay, looks good to me. @bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Aug 4, 2016

📌 Commit 74de967 has been approved by aneeshusa
bors-servo added a commit that referenced this pull request Aug 4, 2016
@bors-servo
Auto merge of #438 - UK992:buildbot-steps, r=aneeshusa
Loading status checks…
aa9171b 
Add description to buildbot steps

Fixes #337

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/saltfs/438)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Aug 4, 2016

⌛ Testing commit 74de967 with merge aa9171b...
@bors-servo
Copy link
Contributor

bors-servo commented Aug 4, 2016

☀️ Test successful - travis
@bors-servo bors-servo merged commit 74de967 into servo:master Aug 4, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@UK992 UK992 mentioned this pull request Aug 23, 2016
Merged
bors-servo added a commit that referenced this pull request Aug 23, 2016
@bors-servo
Auto merge of #472 - UK992:buildbot-steps, r=aneeshusa
Loading status checks…
0b3c861 
Fix buildbot step description for shell commands

This should fix description for shell commands where after #438 shows only "ran", instead of command.
r? @aneeshusa

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/saltfs/472)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@aneeshusa aneeshusa

Labels
S-awaiting-review
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@UK992 @aneeshusa @bors-servo @edunham @jdm
You can’t perform that action at this time.