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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante planned changes to this revision.
Mordante added inline comments.


================
Comment at: libcxx/utils/ci/run-buildbot:10-11
 
 set -ex
 set -o pipefail
 
----------------
Quuxplusone wrote:
> 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).
Thanks for the suggestion. I've started a build to test it. This looks more scalable and doesn't require to change the order so I like this a lot better.
If this works I'll add some comment why this is required.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101303