Skip to content
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

Prevent UnifiedTreeBuilder producing step names over 50 characters in length #293

Merged
merged 1 commit into from
Nov 12, 2024

Conversation

asb
Copy link
Contributor

@asb asb commented Nov 5, 2024

Upstream buildbot releases error if a step name has over 50 characters long buildbot/buildbot#3414. UnifiedTreeBuilder can produce quite long step names that violate this, meaning it's difficult to spin up a local test environment (as in #289) without local hacks to workaround this.

In this patch, we simply truncate step names at creation time. This means llvm-zorg's buildbot config can be used with an unmodified upstream buildbot package.

…ength

Upstream buildbot releases error if a step name has over 50 characters
long <buildbot/buildbot#3414>.
UnifiedTreeBuilder can produce quite long step names that violate this,
meaning it's difficult to spin up a local test environment (as in
<llvm#289>) without local hacks to
workaround this.

In this patch, we simply truncate step names at creation time. This
means llvm-zorg's buildbot config can be used with an unmodified
upstream buildbot package.
@asb
Copy link
Contributor Author

asb commented Nov 5, 2024

Just for reference, these are the errors you get trying to start a buildmaster (or just checkconfig) on upstream buildbot without this fix:

/home/asb/repos/llvm-zorg/myenv/lib/python3.12/site-packages/buildbot/config/master.py:325: ConfigWarning: WARNING: Title is too long to be displayed. "Buildbot" will be used instead.
  warnings.warn('WARNING: Title is too long to be displayed. ' +
Configuration Errors:
  Step NinjaCommand name 'build-docs-llvm-html-docs-clang-html-docs-clang-tools-html-docs-lld-html-docs-lldb-html-docs-flang-html-docs-openmp-html-docs-polly-html' exceeds maximum length of 50
  Step NinjaCommand name 'build-docs-libc-html-docs-libcxx-html-docs-libunwind-html' exceeds maximum length of 50
  Step LitTestCommand name 'test-build-clangd-clangd-index-server-clangd-indexer-check-clangd' exceeds maximum length of 50
  Step NinjaCommand name 'build-docs-llvm-html-docs-clang-html-docs-clang-tools-html-docs-lld-html-docs-lldb-html-docs-flang-html-docs-openmp-html-docs-polly-html' exceeds maximum length of 50
  Step NinjaCommand name 'build-docs-libc-html-docs-libcxx-html-docs-libunwind-html' exceeds maximum length of 50
  Step LitTestCommand name 'test-build-clangd-clangd-index-server-clangd-indexer-check-clangd' exceeds maximum length of 50
  Step NinjaCommand name 'build-docs-llvm-html-docs-clang-html-docs-clang-tools-html-docs-lld-html-docs-lldb-html-docs-flang-html-docs-openmp-html-docs-polly-html' exceeds maximum length of 50
  Step NinjaCommand name 'build-docs-libc-html-docs-libcxx-html-docs-libunwind-html' exceeds maximum length of 50
  Step LitTestCommand name 'test-build-clangd-clangd-index-server-clangd-indexer-check-clangd' exceeds maximum length of 50

asb added a commit to asb/llvm-project that referenced this pull request Nov 5, 2024
Once <llvm/llvm-zorg#289> and
<llvm/llvm-zorg#293> land, it's quite reasonable
to ask people to test their builder configurations locally. This patch
adds documentation on how to do so.

I think review at this stage is useful, but of course if there's more
review feedback on <llvm/llvm-zorg#289> it's
possible some details may change. This won't be committed until those
llvm-zorg PRs land of course.
asb added a commit to asb/llvm-project that referenced this pull request Nov 5, 2024
Once <llvm/llvm-zorg#289> and
<llvm/llvm-zorg#293> land, it's quite reasonable
to ask people to test their builder configurations locally. This patch
adds documentation on how to do so.

I think review at this stage is useful, but of course if there's more
review feedback on <llvm/llvm-zorg#289> it's
possible some details may change. This won't be committed until those
llvm-zorg PRs land of course.
# <https://github.com/buildbot/buildbot/issues/3414>.
def trunc50(name):
if len(name) > 50:
return name[:47] + "..."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to suggest f"{s:.47s}..." but this adds ... regardless of length, so don't do that.

Just mentioning that in case someone else was thinking of that.

asb added a commit that referenced this pull request Nov 8, 2024
This makes it (relatively) easy to test a builder setup locally. The
buildmaster and the web interface should be bound only to local
interfaces for security reasons (you can use ssh port forwarding if
wanting to run on a server).

See llvm/llvm-project#115024 for instructions on how to use, but note #293 is needed to avoid disabling certain builders or needing to hack your local buildbot install.
@asb
Copy link
Contributor Author

asb commented Nov 12, 2024

Ping?

Copy link
Contributor

@DavidSpickett DavidSpickett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Whilst folks might be looking at the step names to see what they do at a glance, if the name is truncated then they can look at the start of the log for that step instead.

(and for complicated builds, it's often better to look at the log file as the step name may not be accurate anyway)

@asb asb merged commit 2a37a78 into llvm:main Nov 12, 2024
2 checks passed
asb added a commit to llvm/llvm-project that referenced this pull request Nov 12, 2024
…#115024)

With <llvm/llvm-zorg#289> and <llvm/llvm-zorg#293> landed, it's now reasonable to ask people to test their builder configurations locally. This patch adds documentation on how to do so.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants