-Werror being used always, not only on validate

[alt-romes]

In 0876e18 there was a refactor of cabal projects which introduced the following "regression":

Previously, we would only compile with ghc-options: -Werror when using the cabal.project.validate{.libonly} projects.
After 0876e18, we always compile with ghc-options: -Werror.

I don't think in-tree builds should use -Werror. It is valuable as a validation tool when the MR has to be finished and be validated by CI, but I don't think that when iterating on a patch one should care about all warnings, and -Werror makes this unnecessarily painful.

cc @philderbeast

[philderbeast]

I'd add "-Wwarn=some-error" in the local project to defer dealing with those.

[alt-romes]

I'd add "-Wwarn=some-error" in the local project to defer dealing with those.

That doesn't seem reasonable. Erroring on a warning in a development build is an exception, not the norm. Having to manually edit the cabal file to ignore warnings doesn't seem like a good developer experience.

[alt-romes]

I don't imagine this is hard to fix. cabal.project.release still exists, so it should suffice to move -Werror from project-cabal/ghc-options.config there.

[philderbeast]

Are you talking about warnings you are introducing?

[alt-romes]

I'm saying that -Werror cannot be used by default on all builds. Only on validate builds.

[Mikolaj]

Are you talking about warnings you are introducing?

I think, Rodrigo is talking about warning instances introduced by the developer, but not necessarily coming from new warning flags introduced by the developer.

[philderbeast]

Sorry I'm going to drop out of this discussion for a bit, on the road in a snow storm!

[philderbeast]

Having to manually edit the cabal file to ignore warnings doesn't seem like a good developer experience.

Ignoring warnings is worse, isn't it?

It bothered me that hackage-security was spamming our build logs with lots of warnings so I fixed those with haskell/hackage-security#306.

I find cabal build output lengthy and its warning easy to miss. I don't mind fixing warnings.

[geekosaur]

Perhaps a flag (e.g. -fdev) possibly in a common import would make sense? Then it could also be used for any other options you'd only want during development.

[alt-romes]

Perhaps a flag (e.g. -fdev) possibly in a common import would make sense? Then it could also be used for any other options you'd only want during development.

I think it would be better not to introduce more configuration levers.

It sounds like Phil is worried that warnings get ignored. I'm not saying otherwise:

• We can/should/already do validate CI with -Werror. So no commits with warnings get merged to master.
• OTOH, we don't need -Werror while developing because it seriously harms iterating quickly on a patch.

This is solved by the mechanism we have used thus far of having a cabal.project.x for different occasions.
One such file is cabal.project.validate which is used in validate jobs. I've updated that one only to use -Werror, reverting the status quo of having -Werror in every project file.

[alt-romes]

This makes it really annoying to use traceShow while developing because you get an error with -Werror...

[philderbeast]

This makes it really annoying to use traceShow while developing because you get an error with -Werror...

@alt-romes I had a look at #9811. I did the same but with traceShowId and found these ways to silence the errors;

$ git diff
diff --git a/cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs b/cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs
index fe2e86d8c..b0d4b87c2 100644
--- a/cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs
+++ b/cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs
@@ -32,6 +32,7 @@ module Distribution.Client.ProjectConfig.Legacy
   , renderPackageLocationToken
   ) where

+import Distribution.Compat.Prelude (traceShowId)
 import Data.Coerce (coerce)
 import Distribution.Client.Compat.Prelude

@@ -268,7 +269,7 @@ parseProjectSkeleton cacheDir httpTransport verbosity projectDir source (Project
         let importLocPath = importLoc `consProjectConfigPath` source

         -- Once we canonicalize the import path, we can check for cyclical imports
-        normLocPath <- canonicalizeConfigPath projectDir importLocPath
+        normLocPath <- canonicalizeConfigPath projectDir $ traceShowId importLocPath

         debug verbosity $ "\nimport path, normalized\n=======================\n" ++ render (docProjectConfigPath normLocPath)

• With a cabal.project.local;

$ cat cabal.project.local
program-options
  ghc-options:
    -Wwarn=unused-imports
    -Wwarn=deprecations
    
$ cabal build cabal-install:cabal

• With command line, specific -Wwarn= options;

$ cabal build cabal-install:cabal --ghc-options="-Wwarn=unused-imports -Wwarn=deprecations"

• With a command line global -Wwarn option;

$ cabal build cabal-install:cabal --ghc-options="-Wwarn"

[alt-romes]

I understand that you can turn specific errors back into warnings again, but that is beyond my point that -Werror is not a good default for development.

The change to use -Werror by default was not agreed upon by the cabal developers.

To make -Werror the default always there needs to be consensus among us and a clear proposal to introduce this change. However, the change slipped through inadvertently into master during the refactor of cabal projects 0876e18. Mistakes are fine, but we need to revert them if they are problematic.

[mpickering]

I agree with Rodrigo that -Werror by default during development is cumbersome to work with. It is common workflow to write code which contains errors to begin with and then clean up all the errors when the patch is ready to submit. Please can we revert this change.

[fendor]

I think -Werror is not a good development default. The described workarounds are easy enough for experts, but newcomer contributors (which we would like to have in cabal) will have a hard time figuring them out. Adding them to a "getting started" page kind of adds noise, modifying sources just so you can develop just like you are used, especially in a project where "run the main" is often the default way to debug.

A -fdev flag has the same disadvantages imo. Not discoverable.

I think -Werror should be enabled in CI only, so we still never have warnings in HEAD, but a more approachable dev environment.

[fendor]

Also, the -Werror change seems orthogonal to the original PR, as it is not mentioned in the PR description, nor the related issues (though, it is prominently mentioned in the commits).

If we want to change this default, there should be an issue where we can discuss the motivation and consequences in more detail.

I have merged #9867 for now, as it restores the status-quo and multiple people have been unexpectedly bitten by the changes. Myself included :D