`CI_CONFIGURE_ARGS=--with-cxxflags` breaks `_wall` configs, and -werror coverage issues

[Dan Bonachea]

We have several *_wall configs in GitLab, whose sole purpose for existence is to globally test with -Wall -pedantic. These are implemented by exporting these flags into envvar $CXXFLAGS before configure.

As a result of configure precedence, GitLab settings like:

• CI_CONFIGURE_ARGS='--with-cxxflags="-DUPCXX_ENABLE_WHATEVER"'

will remove/override the -pedantic -Wall setting from the _wall configs, defeating their purpose (and potentially giving a false sense of security)

I think the right answer is probably to add a CI_CXXFLAGS and encourage its use instead of CI_CONFIGURE_ARGS=--with-cxxflags (possibly even scanning for that pattern with an error?), so the _wall configs can be additive with CI_CXXFLAGS.

Of course the obvious extension would also add a CI_CFLAGS to tweak the C compiles (of which there is very little in UPC++, but plenty in GASNET), although none of our GitLabs are currently setting CFLAGS anyhow so this one is just academic at the moment.

On a related note, the _wall configs set -Wall -pedantic globally, but do NOT appear to be setting -Werror for the library build steps. This means we're only reporting pedantic/wall defects that are visible during test compilation (where -Werror is enabled by default). IOW any pedantic/wall defects in the libupcxx *.cpp files and all of GASNet will appear only in the giant build log, but will not be flagged as a failure and thus probably go unnoticed. If this is the intended behavior, then we can fix the first issue by simply moving -Wall -pedantic from $CXXFLAGS to a CI_EXTRAFLAGS append. However if we actually want to detect/report pedantic/wall defects in libupcxx and GASNet then we might need those configs to also add -Werror to CXXFLAGS (offhand I don't know if that has any negative effects on configure-based warning detection, but it might), or to MANUAL_CXXFLAGS during build steps (which is likely to be safer).

[Paul Hargrove]

Regarding the following

On a related note, the _wall configs set -Wall -pedantic globally, but do NOT appear to be setting -Werror for the library build steps. This means we're only reporting pedantic/wall defects that are visible during test compilation (where -Werror is enabled by default). IOW any pedantic/wall defects in the libupcxx *.cpp files and all of GASNet will appear only in the giant build log, but will not be flagged as a failure and thus probably go unnoticed. If this is the intended behavior, then we can fix the first issue by simply moving -Wall -pedantic from $CXXFLAGS to a CI_EXTRAFLAGS append. However if we actually want to detect/report pedantic/wall defects in libupcxx and GASNet then we might need those configs to also add -Werror to CXXFLAGS (offhand I don't know if that has any negative effects on configure-based warning detection, but it might), or to MANUAL_CXXFLAGS during build steps (which is likely to be safer).

That is the subject of issue 504, in which the most recent "real comment" is

The changes above have been committed, but they fail to add -Werror to the library compile. So, these three jobs may generate new warnings, but not the desired failure. This issue remains open pending additional work to address that.

[Dan Bonachea]

To summarize my thoughts above, I suspect the best path forward is to remove the $CXXFLAGS settings from _wall configs and instead append -Wall -pedantic -Werror -* to both $MANUAL_CXXFLAGS (for library build) and also append it to CI_EXTRAFLAGS (for test build), where -* is any other settings needed to let -pedantic pass on the platform (e.g. -DUPCXX_USE_NODISCARD=0)

[Paul Hargrove]

Resolved (as @Dan Bonachea proposed) in 1de5b26ff of the upcxx-ci repo.