abhina.sreeskantharajan added a comment. In D95246#2519729 <https://reviews.llvm.org/D95246#2519729>, @jhenderson wrote:
> In D95246#2519642 <https://reviews.llvm.org/D95246#2519642>, > @abhina.sreeskantharajan wrote: > >> In D95246#2518989 <https://reviews.llvm.org/D95246#2518989>, @jhenderson >> wrote: >> >>> Sorry, could you revert this please. I don't think this is a good fix, as >>> you've reduced coverage within the test, and some changes are completly >>> redundant/add extra noise. I've commented inline with examples. Skimming >>> D94239 <https://reviews.llvm.org/D94239> suggests that has the same issue. >>> >>> Could you also please explain the messages your system is actually >>> producing so we can provide a better solution than this fix. >>> >>> I'm also concerned that you are doing this to fix this test for a system, >>> yet there are no build bots that report this error. A future contributor is >>> likely to break them in the same way/add new tests with the same issue. If >>> your system is a system that is supposed to be supported by LLVM, there >>> needs to be a build bot. If it isn't supported, you should bring this up on >>> llvm-dev (if you haven't already) to get buy-in for support. >> >> Thanks for the feedback. I've reverted my changes from these two patches. We >> have indicated that we wish to add support for the z/OS platform but we have >> not set up a buildbot yet. >> >> In D95246#2519086 <https://reviews.llvm.org/D95246#2519086>, @grimar wrote: >> >>> As far I understand, looking on the description of D94239 >>> <https://reviews.llvm.org/D94239>, the message on z/OS looks like "EDC5129I >>> No such file or directory.". >>> I guess the `EDC5129I` is a stable error code? So why not to check for a >>> possible `EDC5129I` prefix word instead of `.*`? >>> (The same applies for other possible errors) >> >> As grimar noted, this is indeed the correct error message. "EDC5129I No >> such file or directory." (Note the extra period at the end) >> Based on your feedback, these are the better alternatives that were >> suggested: > > Slightly off-the-wall idea: I'm assuming you don't control your system in > such a way that you can change the error message to omit the error code? Right, I'm not able to change the error message. >> '{{.*N|n}}o such file or directory' >> {{EDC5129I N|N|n}}o such file or directory' >> >> Some testcases fail because of the extra period at the end. For those >> testcases, this is a possible alternative. >> >> {{.*N|n}}o such file or directory{{\.?}} >> >> Please let me know if there are better alternatives I could look into. > > I think you can just omit the trailing full stop in those cases. If the test > isn't using --match-full-lines, it should be fine. If it is, adding `{{.?}}` > seems reasonable. > > Having the error code explicitly in the pattern looks like the right solution > for now, but a thought on that - it seems like tests will still have the > fragility problem for when someone else writes a new test that checks the > message due to the error code not being present on most systems. Is the error > code different for each system error message (I'm guessing it is)? I wonder > if we would be better off adding some sort of lit substitution or similar > that expands to the right string for the given host OS, which could in turn > be fed to FileCheck. It might look a bit like this in practice: > > # RUN: not do-a-thing %t 2>&1 | FileCheck %s -DMSG=%enoent -DFILE=%t > > # CHECK: error: '[[FILE]]': [[MSG]] > > What do you think? I like the lit substitution solution, it will be a lot cleaner compared to a complicated regex. I've noticed there are already different regex for the same error message so this will help the error messages be uniform as well. I can look into implementing this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95246/new/ https://reviews.llvm.org/D95246 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
