cabal v2-configure

[ptkato]

This PR fixes #5591 and also adds a few tests to v2-configure and v2-reconfigure both.

After #7405, this PR now fixes #5591, fixes #7180, fixes #7411, and closes #7405, while also adding tests for this new implementation.

---

Please include the following checklist in your PR:

• Patches conform to the coding conventions.
• Any changes that could be relevant to users have been recorded in the changelog.
• The documentation has been updated, if necessary.

[emilypi]

On first pass, this is looking good.

[phadej]

I still don't understand what you need v2-(re)configure for. See discussion in #7180 (I assume people want to use configure though none of our docs suggest it, do they - let's fix docs!)

I'm 👎 on adding this command.

[Mikolaj]

Still, this #5591 was milestoned and re-milestoned a few times. That's not the same as approval, but quite unfortunate anyway if the issue doesn't make sense. The very fact that #5591 and #7180 are not cross-linked is very unfortunate as well, though understandable in a project of this size with no project managers and devs doing at once coding/management/design/troubleshooting/etc.

[ptkato]

I still don't understand what you need v2-(re)configure for. See discussion in #7180 (I assume people want to use configure though none of our docs suggest it, do they - let's fix docs!)

You're suggesting to, instead of calling on a separate v2-reconfigure command, call instead on v2-configure together with that extra flag? That wouldn't change the workflow of the actual code in this PR much, I suppose that's more of a semantics thingy, and would be easy enough to accomplish, yes.

[phadej]

@ptkato I'm suggesting to not use v2-configure at all, it's a step most people don't ever need.

[emilypi]

@phadej I'm afraid I disagree. If i'm interfacing with other people's projects, and they have a fixed cabal.project, i tend to lean on v2-configure to set my project-local overrides and other settings: -O0, coverage, testing, profiling etc. This is an important workflow. Reconfigure, I could see being a flag that mappends and overwrites and doesn't generate a new cabal.project.local backup, and I think that's useful as well.

I'm curious to know why you think what you think, since it's not really clear from the issue you're citing, and it's not detailed anywhere in any issue. Right now, this MR is a cabal contributors worst nightmare: obscure comments from an issue from last year, not linked to the actual ticket, no comments on the actual ticket, and an immediate thumbs down from you. So, I'd like to hear why that is.

[phadej]

@emilypi yes, your use case is valid. I simply write cabal.project.local by hand, and edit it if I need something. Why I do so? because I see the state when I open an editor, and don't have to remember what I already configured and how and what I didn't.

Your use case is scriptable modification of cabal.project.local, like cabal user-config and I'd be 👍 on making command for that (say cabal package-config). v2-configure and v2-reconfigure do more and give wrong impression that you have to run them to do build, that is not true. Just cabal v2-build.

/to repeat/ v2-configure is not the spiritual successor of v1-configure but too many people think it is, that's why I think v2-configure is bad. (Note how v2-install is not the same as v1-install but in many issues people think they are interchangeable - e.g. for installing libraries).

From #5591,

For old-build we have a useful reconfigure command (originally implemented by @ttuegel), which is like configure, but instead of fully overwriting the old settings file, it mappends the new flags on top of the old ones and records that.

But v2-configure is not like v1-configure. The premise is false.

[phadej]

And addition, more often then not I add head.hackage or some other (local no+index) repository or source-repository-packages, which v2-configure cannot. For head.hackage there a snippet you copy&paste, for source-repository-package you need repository url and commit, supplying either via command line interface is in my opinion non ergonomic.

For optimizations likely there is short -O0 flag. Maybe if there were short flags for --enable-tests --enable-profiling the v2-build interface would be slightly more convenient too. (And you wouldn't write -O0 --enable-profiling by accident, with v2-reconfigure you may get such state - as it's hidden).

[emilypi]

@phadej Alright, so correct me if I'm wrong, but the subtlety is this:

1. v2-configure creates a build plan via dry-run and cabal.project.local with build flags passed in, and this is doing more than just modifying the cabal.project.local. Therefore, this command is redundant with cabal build --dry and <insert .local modification script command here>, and at the moment, we do not have the latter command.

2. A command with the ability to do cabal.project.local modifications can't possibly be fully encapsulated in terms of cabal.project.local options, because there are some flags you can pass to a cabal.project.local file which are not describable as build flags (e.g. your source-repository-package example), and so a command that modifies the local project file would need immediate qualification in the docs, saying what subset of options it does and does not support.

I think i see better now. I was wondering up above why we need to call build and rebuild if all we're doing is mappend'ing the options to the file. This makes more sense. I'll open up an issue today and link against this PR and the relevant issues above and we can take discussion there. @ptkato let's discuss

[emilypi]

@ptkato we have a decision at #7405

[ptkato]

I see, however the plans there are to change multiple behaviours, does the removal of the --dry-run thingy even belong within this PR? I personally don't mind to sneak it in here, by the by.

[ptkato]

Hello there, requesting review

@emilypi @fgaz @Mikolaj

[Mikolaj]

Hey, congrats on completing the PR. I was the least engaged (except for trolling users), but if others turn out too busy, I will gladly review.

Re "Merging master", I don't want to be a rebase nazi, but would it be feasible to rebase instead of merging? I know with long running PRs and complex histories it's no longer easy to rebase, but perhaps here we can afford that luxury?

[ptkato]

Re "Merging master", I don't want to be a rebase nazi, but would it be feasible to rebase instead of merging? I know with long running PRs and complex histories it's no longer easy to rebase, but perhaps here we can afford that luxury?

Done.

[Mikolaj]

Done.

Yay, thank you. :)

[ptkato]

I could even squash those earlier commits, since they are outdated, now that the implementation changed.

Actually, no, it might be good to leave those alone, since they will contextualise the later commits.

[Mikolaj]

Yep, I like dirty history, too, just not tangled history. :)

[emilypi]

Looks great @ptkato. Do you mind opening up an issue in conjunction with this PR for stripping the builds/rebuilds from configure? I think that's the only step I'd follow up with. Other than that, we're free to bikeshed whether --append is the correct choice of name. personally, it makes sense, but i'd like to hear what others think.

PS: Let's get CI green.

[ptkato]

Other than that, we're free to bikeshed whether --append is the correct choice of name. personally, it makes sense, but i'd like to hear what others think.

About that, the way it is set up, it will add two commands for each one: --enable-append, --disable-append, --enable-overwrite, and --disable-overwrite. Regarding the actual names, I think --append is fine, however --overwrite is terrible and quite ambiguous, maybe something like --no-backup (or something of the sort) might be better.

[ptkato]

Just for the record, that sack of commits there is because I accidentally had one of the merge commits included in the rebase without noticing it. Now it is correct.

[ptkato]

Oh, I didn't know rebase has this interaction with git bisect. I take full blame for advocating rebase over merge. If for any reason merge seems better in a particular case, please feel free to merge manually --- the github UI setting are meant to help the devs by indicating an aspirational preference, but not to hinder them by outlawing the other choices. :)

Wait, is that so? I thought we were rebasing only the local changes, and then the PR itself would be merged into master.

[fgaz]

Yeah, the merge strategy in the github repo settings was recently changed, this is what I see:

[Mikolaj]

I'm fine with merging a series of commits, as long as they are fully rebased themselves (this is equivalent to git rebase; git merge --no-fast-forward --- normally merge commit is not created if things can be fast-forwarded). Is there a way to suggest this workflow is welcome and, at the same time, enforce that merged commits are fast-forward? I know there is in bitbucket, but I don't know about git.

Alternatively, would we get the same result if you, @ptkato, did git merge --no-fast-forward and then clicked the github button?

[phadej]

FWIW, rebase and merge also doesn't leave a pointer to the original PRs in the git commit messages, so you are more or less forced to use GitHub UI to find which PR commit originated from.

I'm not a fan,

[Mikolaj]

You are right, when the author does a manual git merge --no-ff, there is no guarantee the PR number would end up in the commit message (I guess git only suggests to put the branch name there or not even that?). I forgot github inserts the nice Merge pull request #NN from <branch>.

OK, I think I was wrong suggesting to remove the github-merge option. If so, squash is probably fine, too. However, can we automatically warn the PR author when the branch is not fully rebased (or even block merging until then)?

[fgaz]

Looks good, but the overwrite help needs fixing, or we could change the flag. Also some minor suggestions

[Mikolaj]

Regarding merge buttons, I've got a helpful suggestion on IRC and came up with a plan. Does it make sense? @fgaz, @emilypi, @ptkato, others?

10:42 < mikolaj> hsyl20: thank you! what I'd like to do is bring back "allow merge commits", but avoid non-trivial merges performed by github --- it seems "Require branches to be up to date before merging" does the job
10:43 < mikolaj> "require linear history" forbids even fast-forward merges via the github button at least and perhaps at all
10:44 < mikolaj> I'm inclied to bring back all the three merge buttons, check "Require branches to be up to date before merging" and, while we are at, increase the number of required reviews from 1 to 2, matching our culture (informal
policy)
10:44 < mikolaj> unless we make exceptions for trivial PRs and require only one review?
10:45 < mikolaj> any objections?
10:48 < mikolaj> this setup has the drawback that PR authors are not warned against manually merging master into their branch if they think rebase would be sufficient (verifying and expressing/exposing the assumption this way), but I
think at least the github buttons don't automatically do non-trivial merges any more, which is even more risky

[Mikolaj]

There were no objections, so I've made the following changes to github settings: for master branch only "Require branches to be up to date before merging" and 2 reviews required (from 1) and globally again all 3 three merge buttons permitted (instead of one). At least the buttons are again active in this PR.

[fgaz]

Oh oops that didn't prompt for a comment. Well:

Looks good, but I wonder if we should hide the new flags for other commands. eg. they don't make sense for build or test. On the other hand we do have a lot of other flags that don't have an effect, so is one more bad?

[emilypi]

Great job @ptkato! Do you mind following this PR up with another issue addressing @phadej's comments? Thanks

[emilypi]

@fgaz i think @ptkato should open another issue for pruning unused commands. I'd rather see this done in one sweep rather than piecemeal, so we can go down the checklist once.