Don't send non-existent extra-lib-dirs to ghc

[gbaz]

Filters extra-lib-dirs before passing to GHC. Resolves #6492

• Any changes that could be relevant to users have been recorded in the changelog.

[Mikolaj]

@gbaz: is this ready for review?

[gbaz]

Sure!

[Mikolaj]

Hey, why is there no label, in that case? :)

[Mikolaj]

This seems safe (famous last words). Is there a cheap way of protecting this PR from accidental reversion via a test?

[Ericson2314]

Should extra-include-dirs be handled in the same way for consistency? I don't love making an ad-hoc policy to work around what's arguably a GHC bug, but if we do this everywhere then it's not ad-hoc? :D

[andreasabel]

I wonder about the placement in Distribution.Simple.GHC. Is this something GHC specific or shouldn't these non-existing directories be filtered out earlier in the pipeline, like after parsing?
Also, I agree with @Ericson2314 's concern that non-existing include directories should also be filtered out.

[gbaz]

There is no place to filter these things out earlier in the pipeline uniformly afaik, because the parsing phases are pure. We only hit IO on ghc invocation.

I looked into doing it for includes, but the place includes are set to send to ghc is elsewhere in the pipeline, and so i think it would be a rather invasive and fragile change compared to this, and also one with no particular purpose other than symmetry. So I'd rather not.

[andreasabel]

Fine with me.

[andreasabel]

@Mikolaj : I can't remember whether the policy was to (a) squash the commit with the changelog into one commit with the feature or (b) to keep it separate.
I guess (a), but feel free to change the label if (b) is the rule. The contributor guide softly recommends (a):

cabal/CONTRIBUTING.md

Lines 240 to 242 in 7babda9

	If your pull request consists of several commits, consider using `squash+merge
	me` instead of `merge me`: the Mergify bot will squash all the commits into one
	and concatenate the commit messages of the commits before merging.

[Mikolaj]

The contributor guide softly recommends (a)

I think it may be recommending it only because squashing requires one more mental action and users devs may need the nudge. If commits are well made (each one compiles, they separate refactorings from changes, etc.), as opposed to a string of fix-up commits, IMHO it's beneficial to retain them, including the changelog commit.

[Mikolaj]

Ultimately it's up to the user and, normally, the users sets the label.

[Mikolaj]

Doh, not user, but dev, PR author.

[andreasabel]

The contributor guide softly recommends (a)

I think it may be recommending it only because squashing requires one more mental action and users devs may need the nudge. If commits are well made (each one compiles, they separate refactorings from changes, etc.), as opposed to a string of fix-up commits, IMHO it's beneficial to retain them, including the changelog commit.

You could add something like this to CONTRIBUTING.md.

[Mikolaj]

You could add something like this to CONTRIBUTING.md.

I'd like to keep CONTRIBUTING.md short and not (too much) opinionated. Perhaps just reduce the nudge? Or focus it on the most obvious case?

"If your pull request consists of several fix-up commits (not every commit compiles or they just don't help in understanding the PR, e.g., by separating formatting, refactoring and real functionality changes), please use squash+merge me instead of merge me..."

[ulysses4ever]

I think we should lean to squashing unless the different commits are really different changes. (a) it makes for a clearer history (I doubt anyone benefits from tens or hundreds of commits "add changelog"), (b) individual commits are not tested by the ci, only the overall PR is, isn't it? in that case, there may be unbuildable "middle" commits that will severely complicate any future git-bisecting.

[Mikolaj]

Right, different trade-offs and tastes. Given that the PR page retains the original commits, I'd not worry about separating formatting, refactoring and (multiple) changes. However, we may want to migrate off github in the future and I'm can't be sure the PR commits are going to be migrated as well (given that the feature branches is deleted). Perhaps let's leave it as is. :)