Add compile-time detection of Ninja-created relative paths

claremacrae commented 4 years ago

I'm using this PR to track experiments in making Approval Tests fail at compile-time if the __FILE__ macro has given incomplete paths, typically because the Ninja generator was used.

See #74 and #55 for background.

claremacrae commented 4 years ago

The code in 00b1334 shows that the compile-time checks do successfully compile on the pre-existing builds, and correctly fail to compile on Ubuntu and macOS when Ninja is used.

The Ubuntu error message is as intended: https://github.com/approvals/ApprovalTests.cpp/runs/374785735

In file included from ApprovalTests/../ApprovalTests/integrations/catch/Catch2Approvals.h:6:0,
                 from tests/Catch2_Tests/ApprovalTests.hpp:6,
                 from tests/Catch2_Tests/main.cpp:9:
ApprovalTests/../ApprovalTests/integrations/CheckFileMacroIsAbsolute.h:8:1: error: static assertion failed: 
ApprovalTests requires an absolute path to source file locations - not the relative one given by this build.
A common cause of this problem is using Ninja with in-tree builds.
Please see https://github.com/approvals/ApprovalTests.cpp/blob/master/doc/TroubleshootingMisconfiguredBuild.md#top
for help.

 static_assert(__FILE__[0] == '/',

The macOS error message shows all the quotes and \ns though https://github.com/approvals/ApprovalTests.cpp/runs/374785748

In file included from ApprovalTests/../ApprovalTests/integrations/catch/Catch2Approvals.h:6:
ApprovalTests/../ApprovalTests/integrations/CheckFileMacroIsAbsolute.h:8:1: error: static_assert failed due to requirement '"ApprovalTests/../ApprovalTests/integrations/CheckFileMacroIsAbsolute.h"[0] == '/'' "\nApprovalTests requires an absolute path to source file locations - not the relative one given by this build.\nA common cause of this problem is using Ninja with in-tree builds.\nPlease see https://github.com/approvals/ApprovalTests.cpp/blob/master/doc/TroubleshootingMisconfiguredBuild.md#top\nfor help.\n\n"
static_assert(__FILE__[0] == '/',
^             ~~~~~~~~~~~~~~~~~~
1 error generated.

claremacrae commented 4 years ago

I tried using raw-string literals in the static_assert message, for nicer output...

On my Mac, the error is good:

../ApprovalTests/../ApprovalTests/integrations/CheckFileMacroIsAbsolute.h:8:27: error: static assertion failed: 
********************************************************************************
* Welcome to Approval Tests.
*
* There seems to be a problem with your build configuration:
* we cannot find the test source file.
*
* ApprovalTests requires an absolute path to source file locations - not the
* relative one given by this build.
*
* A common cause of this problem is using Ninja with in-tree builds.
*
* For details on how to fix this, please visit:
* https://github.com/approvals/ApprovalTests.cpp/blob/master/doc/TroubleshootingMisconfiguredBuild.md
********************************************************************************

    8 | static_assert(__FILE__[0] == '/',

On Ubuntu with GitHub Actions, this works, although the user does need to know to scroll down in the output:

https://github.com/approvals/ApprovalTests.cpp/runs/374811041

ApprovalTests/../ApprovalTests/integrations/CheckFileMacroIsAbsolute.h:8:1: error: static assertion failed: 
********************************************************************************
* Welcome to Approval Tests.
*
* There seems to be a problem with your build configuration:
* we cannot find the test source file.
*
* ApprovalTests requires an absolute path to source file locations - not the
* relative one given by this build.
*
* A common cause of this problem is using Ninja with in-tree builds.
*
* For details on how to fix this, please visit:
* https://github.com/approvals/ApprovalTests.cpp/blob/master/doc/TroubleshootingMisconfiguredBuild.md
********************************************************************************

 static_assert(__FILE__[0] == '/',

With Apple Clang, it's still ugly and fairly unreadable:

In file included from ApprovalTests/../ApprovalTests/integrations/catch/Catch2Approvals.h:6:
ApprovalTests/../ApprovalTests/integrations/CheckFileMacroIsAbsolute.h:8:1: error: static_assert failed due to requirement '"ApprovalTests/../ApprovalTests/integrations/CheckFileMacroIsAbsolute.h"[0] == '/'' "\n********************************************************************************\n* Welcome to Approval Tests.\n*\n* There seems to be a problem with your build configuration:\n* we cannot find the test source file.\n*\n* ApprovalTests requires an absolute path to source file locations - not the\n* relative one given by this build.\n*\n* A common cause of this problem is using Ninja with in-tree builds.\n*\n* For details on how to fix this, please visit:\n* https://github.com/approvals/ApprovalTests.cpp/blob/master/doc/TroubleshootingMisconfiguredBuild.md\n********************************************************************************\n"
static_assert(__FILE__[0] == '/',
^             ~~~~~~~~~~~~~~~~~~
1 error generated.

claremacrae commented 4 years ago

With Apple Clang, it's still ugly and fairly unreadable:

In file included from ApprovalTests/../ApprovalTests/integrations/catch/Catch2Approvals.h:6:
ApprovalTests/../ApprovalTests/integrations/CheckFileMacroIsAbsolute.h:8:1: error: static_assert failed due to requirement '"ApprovalTests/../ApprovalTests/integrations/CheckFileMacroIsAbsolute.h"[0] == '/'' "\n********************************************************************************\n* Welcome to Approval Tests.\n*\n* There seems to be a problem with your build configuration:\n* we cannot find the test source file.\n*\n* ApprovalTests requires an absolute path to source file locations - not the\n* relative one given by this build.\n*\n* A common cause of this problem is using Ninja with in-tree builds.\n*\n* For details on how to fix this, please visit:\n* https://github.com/approvals/ApprovalTests.cpp/blob/master/doc/TroubleshootingMisconfiguredBuild.md\n********************************************************************************\n"
static_assert(__FILE__[0] == '/',
^             ~~~~~~~~~~~~~~~~~~
1 error generated.

It does at least wrap in the GitHub Actions output, so there, the user doesn't need to scroll to the right endlessly.

claremacrae commented 4 years ago

The static_assert output in Visual Studio 2019 with Ninja is:

  In file included from ..\ApprovalTests\..\ApprovalTests/integrations/catch/Catch2Approvals.h:6:
D:\Users\Clare\Documents\Programming\github\ApprovalTests\ApprovalTests.cpp\ApprovalTests\integrations\CheckFileMacroIsAbsolute.h(5,1): error : static_assert failed due to requirement '"..\\ApprovalTests\\..\\ApprovalTests/integrations/CheckFileMacroIsAbsolute.h"[1] == ':'' "\n********************************************************************************\n* Welcome to Approval Tests.\n*\n* There seems to be a problem with your build configuration:\n* we cannot find the test source file.\n*\n* ApprovalTests requires an absolute path to source file locations - not the\n* relative one given by this build.\n*\n* A common cause of this problem is using Ninja with in-tree builds.\n*\n* For details on how to fix this, please visit:\n* https://github.com/approvals/ApprovalTests.cpp/blob/master/doc/TroubleshootingMisconfiguredBuild.md\n********************************************************************************\n"
  static_assert(__FILE__[1] == ':',
  ^             ~~~~~~~~~~~~~~~~~~
  1 error generated.

Whilst Visual Studio does wrap the text, it is still hard to read. So I'll shorten the text to a single line, to prevent there being quotes and \n characters shown to developers.

claremacrae commented 4 years ago

The help messages are now:

Linux (gcc) https://github.com/approvals/ApprovalTests.cpp/runs/375831586

n file included from ApprovalTests/../ApprovalTests/integrations/catch/Catch2Approvals.h:6:0,
                 from tests/Catch2_Tests/ApprovalTests.hpp:6,
                 from tests/Catch2_Tests/main.cpp:9:
ApprovalTests/../ApprovalTests/integrations/CheckFileMacroIsAbsolute.h:11:1: error: static assertion failed: There seems to be a problem with your build configuration, probably with Ninja. Please visit https://github.com/approvals/ApprovalTests.cpp/blob/master/doc/TroubleshootingMisconfiguredBuild.md
 static_assert(__FILE__[0] == '/',
 ^~~~~~~~~~~~~

Mac (clang) https://github.com/approvals/ApprovalTests.cpp/runs/375831610

In file included from tests/Catch2_Tests/main.cpp:9:
In file included from tests/Catch2_Tests/ApprovalTests.hpp:6:
In file included from ApprovalTests/../ApprovalTests/integrations/catch/Catch2Approvals.h:6:
ApprovalTests/../ApprovalTests/integrations/CheckFileMacroIsAbsolute.h:11:1: error: static_assert failed due to requirement '"ApprovalTests/../ApprovalTests/integrations/CheckFileMacroIsAbsolute.h"[0] == '/'' "There seems to be a problem with your build configuration, probably with Ninja. Please visit https://github.com/approvals/ApprovalTests.cpp/blob/master/doc/TroubleshootingMisconfiguredBuild.md"
static_assert(__FILE__[0] == '/',
^             ~~~~~~~~~~~~~~~~~~
1 error generated.

Windows - VS2019 local build (text is wrapped in Output window)

D:\Users\Clare\Documents\Programming\github\ApprovalTests\ApprovalTests.cpp\ApprovalTests\integrations\CheckFileMacroIsAbsolute.h(9,1): error : static_assert failed due to requirement '"..\\ApprovalTests\\..\\ApprovalTests/integrations/CheckFileMacroIsAbsolute.h"[1] == ':'' "There seems to be a problem with your build configuration, probably with Ninja. Please visit https://github.com/approvals/ApprovalTests.cpp/blob/master/doc/TroubleshootingMisconfiguredBuild.md"
  static_assert(__FILE__[1] == ':',
  ^             ~~~~~~~~~~~~~~~~~~
  1 error generated.

claremacrae commented 4 years ago

@jwillikers if you have time, I'd be really grateful if you could review the changes in this PR please?

As far as I can tell, this should enable compile-time failure on all platforms for Ninja-in-tree builds, for Catch2, doctest and Google Test.

In my experiments, [BOOST].UT also fails to set a full path for source_location for Ninja in-tree builds, but I was unable to find a compile-time way of detecting this, so with this test framework, the original run-time check will kick in. (I'll improve its error message as part of #74)

Thanks in advance for any feedback you can offer....

jwillikers commented 4 years ago

@claremacrae I will review the PR today.

claremacrae commented 4 years ago

Things I'd like to do to close this PR:

[x] Add mingw/MSYS CI build
[x] Add CygWin CI build
[x] Test with 'MSYS Makefiles' CMake generator
[x] Based on the results of the two new CI builds, review the #ifdef for detecting full paths in __FILE__ in Windows
[x] Re-read all the comments to make sure I've not missed anything out
[x] Decide what to do about MSYS accepting both windows and unix-style paths - see this comment
[x] Review all the differences on branch
[ ] Accept this PR

General Definition of Done actions:

[x] Tests
[ ] Documentation
- [ ] Sample code
- [ ] Copy+pastable template (if appropriate)
- [ ] A link on Features
- [x] A link on build/relnotes_X.X.X.md
- [ ] Links on other appropriate places
[x] Check the Dashboard (after pushing)

claremacrae commented 4 years ago

This is old, but describes minnow path handling http://mingw.org/wiki/Posix_path_conversion

approvals / ApprovalTests.cpp

Add compile-time detection of Ninja-created relative paths #79