This is an archive of the discontinued LLVM Phabricator instance.

[bazel] Remove terminfo dependency
ClosedPublic

Authored by aaronmondal on May 19 2023, 4:19 PM.

Details

Summary

The only enabling configuration of this is irreproducible. Since
terminfo doesn't provide essential functionality, remove it so that all
external dependencies now follow the same config_setting pattern.

This should be an NFC for most setups and all CI setups.

Diff Detail

Event Timeline

aaronmondal created this revision.May 19 2023, 4:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 19 2023, 4:19 PM
aaronmondal requested review of this revision.May 19 2023, 4:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 19 2023, 4:19 PM
aaronmondal added a reviewer: Restricted Project.May 19 2023, 4:19 PM
aaronmondal added a project: Restricted Project.

You say that it doesn't provide essential functionality. Doesn't that mean it should be an optional dep, perhaps defaulting to disabled, rather than deleting terminfo entirely?

terminfo is used by a few tools such as lldb and clang-repl. The most significant user is lldb, which Bazel doesn't have a port yet. For the other tools, I agree that they "doesn't provide essential functionality,"

When we add lldb, the dependency will be significant...

You say that it doesn't provide essential functionality. Doesn't that mean it should be an optional dep, perhaps defaulting to disabled, rather than deleting terminfo entirely?

For llvm/Support/Unix/Process.inc I question whether terminfo is necessary at all. That code looks hacky in the first place and we already have fallbacks to TERM that I'd imagine to basically accomplish the same thing, but without locks and extern C-to-avoid-searching-for-headers.

terminfo is used by a few tools such as lldb and clang-repl. The most significant user is lldb, which Bazel doesn't have a port yet. For the other tools, I agree that they "doesn't provide essential functionality,"

Hmm we'd definitely want lldb in the overlay at some point. The current implementation of the terminfo dep in Bazel isn't reproducible though. We'd need an "external" terminfo/curses for lldb. The current build files only support "disable" and "system" configurations.

AFAICS one would usually build ncurses to get terminfo, right? terminfo in lldb is also only relevant if curses is present. Would it make sense to create an ncurses build file instead of the terminfo checks?

MaskRay accepted this revision.May 22 2023, 8:41 AM

Removing terminfo seems fine. When we need it, we can do llvm_zlib and llvm_zstd style dependencies...

This revision is now accepted and ready to land.May 22 2023, 8:41 AM
phosek accepted this revision.May 22 2023, 9:10 AM

LGTM

This revision was automatically updated to reflect the committed changes.

Hi! Just FYI "bazel build" tag was created so that interested people will run bazel build as part of their pre-merge check, not in reviewing changes to bazel. It's also quite large.

Hi! Just FYI "bazel build" tag was created so that interested people will run bazel build as part of their pre-merge check, not in reviewing changes to bazel. It's also quite large.

Given the interest in keeping up with bazel related changes I think it would be nice to add a "bazel change" tag. WDYT?

Hi! Just FYI "bazel build" tag was created so that interested people will run bazel build as part of their pre-merge check, not in reviewing changes to bazel. It's also quite large.

Given the interest in keeping up with bazel related changes I think it would be nice to add a "bazel change" tag. WDYT?

I'd also appreciate that. The "upkeep/fix" changes are probably not too interesting, but it would be very nice to have a tag for actual build logic changes such as this commit.

utils/bazel/llvm-project-overlay/llvm/BUILD.bazel