[libcxx-commits] [PATCH] D101303: [WIP][libc++][DI] Fix check-generated-output.

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone requested changes to this revision.
Quuxplusone added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/utils/ci/run-buildbot:10-11
 
 set -ex
 set -o pipefail
 
----------------
Mordante wrote:
> Mordante wrote:
> > Quuxplusone wrote:
> > > @Mordante: Are you sure the script lets non-ASCII characters pass? My intent/understanding had been that `set -e` makes the build fail if any of these greps return nonzero; and I believe that that's what I was observing when I made the PR originally.
> > Yes I noticed the non-ASCII hyphen in some comment which was copied from the Standard. So I was surprised the build passed.
> > An example is this build https://buildkite.com/llvm-project/libcxx-ci/builds/2807#2ec7bc42-3bac-4a7b-98ac-c3b8d03390c7 
> If you have a better suggestion how to solve the issue, I'm open to suggestions.
Oh, gross.
https://unix.stackexchange.com/questions/65532/why-does-set-e-not-work-inside-subshells-with-parenthesis-followed-by-an-or/65564
https://stackoverflow.com/questions/57681955/set-e-does-not-respect-logical-not
The `!` is flipping the exit code from 1 to 0 or vice versa (as expected) but //also// disabling the effects of `set -e` (not expected). Compare the effects of running this script:
```
set -e
false
echo hello  # never reached
```
versus this one:
```
set -e
! true
echo hello  # oops, yes reached
```
So, I'm down this rabbit hole now.
I don't want to just chain everything together with `&&`, though; that seems unscaleable, in that if anyone ever forgets to end a line with `&&`, things will silently break again. I think the right solution is to find the appropriate replacement for `!`.

Could you try this out?

```
    # Check if the diffs are empty, fail otherwise.
    ! grep -q '^--- a' ${BUILD_DIR}/generated_output.patch || false
    # Reject patches that introduce non-ASCII characters or hard tabs.
    ! grep -rn '[^ -~]' libcxx/include/ || false
    # Check that no dependency cycles have been introduced.
    python3 libcxx/utils/graph_header_deps.py >/dev/null
```

and obviously with a commit message that explains why the seemingly pointless `|| false` is needed (if I'm right that it works at all, that is).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101303/new/

https://reviews.llvm.org/D101303