Don't set -march=native and -Werror by default

[AMDmi3]

See https://amdmi3.ru/posts/packaging-friendliness-compiler-flags-to-avoid/

[Quuxplusone]

FWIW, that blog post seems mistaken to me: majorly re -Werror and minorly re -march=.

The post's argument re -Werror is "This flag makes compiler reject code with warnings, forcing author to fix all of them and thus write cleaner code, so it does make sense to enable it for local builds and for continuous integration. However..." But that's literally what we're doing here! We enable -Werror for local builds and for CI. If you've got a clever way to keep -Werror for local builds and CI, while snipping it out for OS package-manager distribution builds specifically, then that would be a cool idea; but I would expect that the package-distribution people generally know how to maintain that one-line patch.

In fact, that blog post literally gives an example of a FreeBSD Ports Collection maintaining such a one-line patch for mpy-cross! Yes! That's exactly how I expect it to work.

The post's argument re -march=native seems to confuse -march= with -mtune=. I'd agree that -mtune=native is harmful, and I suppose -march=native probably isn't helpful, but I don't have the impression that -march=native is actually harmful.

So, I strongly recommend keeping -Werror; and I'm neutral on -march=native. (Dropping -Werror would be catastrophic for code quality. Dropping -march= would at worst make the program run a bit slower.)

[AMDmi3]

If you've got a clever way to keep -Werror for local builds and CI, while snipping it out for OS package-manager distribution builds specifically, then that would be a cool idea

You don't need that, the same way as you don't need to forcibly run linters, profilers, enable code instrumentation and run tests for everybody who just wants to compile your code. Just add these to CI, and anybody is free to enable these locally,or not. If they don't while they should (as in for submitting code upstream), the CI will detect that.

In fact, that blog post literally gives an example of a FreeBSD Ports Collection maintaining such a one-line patch for mpy-cross! Yes! That's exactly how I expect it to work.

It's the opposite of how it should work. Upstream should provide code which compiles out of box without any special tuning and crafting, and especially without pitfalls which may go unnoticed when creating a port/package and then suddenly break it for everybody after seemingly unrelated changes. Modifying upstream code in the package is only tolerable for local (such as FreeBSD-specific) changes, which upstream cannot properly test and maintain and/or which unnecessary complicate generic code. All other changes should be upstreamed to benefit all users of the code and to simplify ecosystem in general. -Werror affects everybody, and it's absolutely impractical to axe it out in each and every package of hyperrogue.

The post's argument re -march=native seems to confuse -march= with -mtune=

In fact, you do.

       -march=cpu-type
           Generate instructions for the machine type cpu-type.  In contrast
           to -mtune=cpu-type, which merely tunes the generated code for the
           specified cpu-type, -march=cpu-type allows GCC to generate code
           that may not run at all on processors other than the one indicated.
                ^^^^^^^^^^^

[Quuxplusone]

Okay, removing -march=native sounds good to me.

I'd still like to see a clever way to enable -Werror in all the cases we do want it (dev builds, CI builds), without the current simple approach of just putting it in the build flags in the Makefile. Removing it wholesale is a bad idea. You write:

Just add these to CI

as if you think it's simple; can you show what that would look like, in terms of this PR?

[AMDmi3]

as if you think it's simple; can you show what that would look like, in terms of this PR?

Won't something like this be enough?

diff --git .github/workflows/build.sh .github/workflows/build.sh
index 0d4afab..5dfa741 100755
--- .github/workflows/build.sh
+++ .github/workflows/build.sh
@@ -17,6 +17,7 @@ fi

 export CC=$GH_COMPILER
 export CXX=${CC%cc}++
+export CXXFLAGS_EARLY=-Werror

 if [[ "$GH_BUILDSYS" == "makefile" ]]; then
   make
diff --git .travis.yml .travis.yml
index 95516a0..525f577 100644
--- .travis.yml
+++ .travis.yml
@@ -162,6 +162,7 @@ script:
   fi
 - |-
   # Build hyperrogue.
+  export CXXFLAGS_EARL=-Werror
   if [[ "$TRAVIS_BUILD_SYSTEM" == "Makefile" ]]; then
     make
   elif [[ "$TRAVIS_BUILD_SYSTEM" == "mymake" ]]; then

[Quuxplusone]

as if you think it's simple; can you show what that would look like, in terms of this PR?

Won't something like this be enough?

I don't know; try making a PR with that, and seeing what the CI thinks of it. :)

[zenorogue]

Merged. Also removed -Werror and -march=native from mymake; these options can be put on the mymake commandline if you want them.