Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use package groups (backport #9565) #10006

Merged
merged 8 commits into from
May 13, 2024
Merged

Use package groups (backport #9565) #10006

merged 8 commits into from
May 13, 2024

Conversation

mergify[bot]
Copy link
Contributor

@mergify mergify bot commented May 13, 2024

We have a lot of projects and they each have packages. Borrowing an idea from Updo, we could group packages and import these groups, fixes #9622. I've also:

  • Removed the coverage and weeder projects.
  • Except for the meta project, projects import package groups.
  • The release project now builds with --index-state="hackage.haskell.org HEAD with ghc-9.8.1 (Build cabal-dev-scripts with ghc-9.8.1 #9600 does more in relation to this). A spin off from this work is the CI check of cabal.project.release in Add a CI check of cabal.project.release? #9601.
  • Projects that have additional constraints, all import the same constraints.
  • Projects that set ghc-options, all import the same ghc-options.
  • Projects that need ghc-latest, all import the same config for this.

Template Β: This PR does not modify cabal behaviour (documentation, tests, refactoring, etc.)

Include the following checklist in your PR:


This is an automatic backport of pull request #9565 done by [Mergify](https://mergify.com).

- Move constraints to project-cabal/constraints.config
- Remove duplicate package groups
- Groups coverage, doctest and validate are the same as the default group
- Remove duplicate validate-libonly package group
- Both validate and validate-libonly are the same
- Move Cabal & Cabal-syntax to pkgs/cabal.config
- Add install package group
- Add benchmarks package group
- Add tests package group
- Both default and libonly groups were the same set of packages that I renamed to tests
- Put install group before tests and benchmarks
- Don't repeat packages
- Add TODO and REVIEW comments on allow-newer exceptions
- Move latest.ghc configuration
- Put program-options first & separate imports
- Remove optional-packages
- Don't reiterate default value for tests and benchmarks
- Don't reiterate default value for optimization
- Add ghc-options.config
- Rename to ghc-latest.config
- Use -Werror in ghc-options.config
- Don't include ghc-options.config for doctest project
- Add project-config/pkgs.config
- Add project-config/pkgs.config importing all package groups
- Move Cabal-described to the cabal package group
- Remove cabal.project.buildinfo
- Have cabal.project.doctest import cabal.project
- Split integration tests into their own package group.
- Add back trailing newlines at EOF in projects
- Integration-tests.config needed for libonly
- Add a README for projects.

(cherry picked from commit 0876e18)

# Conflicts:
#	cabal.project.release
- Satisfy -Wmissing-signatures in test-runtime-deps
- Satisfy -Wx-partial in HackageBenchmark
- Satisfy -Wunused-imports in QuickCheck.Instances.Cabal
- Use partial pattern for filtering in list comprehension
- Don't error on deprecated import

(cherry picked from commit 8453ee0)
- Reduce duplication in test flag setup
- Avoid "New config" picking cabal's own project

Before this change, this was the test failure:

Unit Tests
  UnitTests.Distribution.Client.Configure
    Configure tests
      New config:                     FAIL
        Exception: project-cabal/pkgs/cabal.config: withBinaryFile: does not exist (No such file or directory)
        Use -p '/New config/' to rerun this test only.
      Replacement + new config:       OK
      Old + new config:               OK
      Old + new config, no appending: OK
      Old + new config, backup check: OK
      Local program options:          OK

1 out of 6 tests failed (0.02s)

(cherry picked from commit d336275)
* Remove cabal.project.libonly

It was only referenced once in a stale Makefile comment about doctests.

* Remove weeder

- remove weeder's configuration
- remove its recipe from Makefile
- remove its project

* Delete cabal.project.doctest

- Adding --ghc-options="-Wwarn" is sufficient to avoid the numerous <interactive> failures seen otherwise
- write-ghc-environment-files has a default of never

(cherry picked from commit 72be26b)
@mergify mergify bot added the conflicts label May 13, 2024
Copy link
Contributor Author

mergify bot commented May 13, 2024

Cherry-pick of 0876e18 has failed:

On branch mergify/bp/3.12/pr-9565
Your branch is up to date with 'origin/3.12'.

You are currently cherry-picking commit 0876e187a.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   Makefile
	modified:   cabal.project
	deleted:    cabal.project.buildinfo
	modified:   cabal.project.doctest
	modified:   cabal.project.libonly
	modified:   cabal.project.meta
	modified:   cabal.project.validate
	modified:   cabal.project.validate.libonly
	modified:   cabal.project.weeder
	new file:   project-cabal/README.md
	new file:   project-cabal/constraints.config
	renamed:    cabal.project.latest-ghc -> project-cabal/ghc-latest.config
	new file:   project-cabal/ghc-options.config
	new file:   project-cabal/pkgs.config
	new file:   project-cabal/pkgs/benchmarks.config
	new file:   project-cabal/pkgs/buildinfo.config
	new file:   project-cabal/pkgs/cabal.config
	new file:   project-cabal/pkgs/install.config
	new file:   project-cabal/pkgs/integration-tests.config
	new file:   project-cabal/pkgs/tests.config

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   cabal.project.release

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

@mergify mergify bot mentioned this pull request May 13, 2024
1 task
@mergify mergify bot added the backport label May 13, 2024
@ulysses4ever ulysses4ever requested a review from Kleidukos May 13, 2024 15:18
@Kleidukos Kleidukos added squash+merge me Tell Mergify Bot to squash-merge and removed conflicts labels May 13, 2024
@mergify mergify bot merged commit edc131f into 3.12 May 13, 2024
40 of 41 checks passed
@mergify mergify bot deleted the mergify/bp/3.12/pr-9565 branch May 13, 2024 16:22
@Mikolaj
Copy link
Member

Mikolaj commented May 14, 2024

@ulysses4ever, @Kleidukos: isn't this PR affected by #9901? Do you plan to revert the problematic portion of the PR, as described in the ticket?

@philderbeast
Copy link
Collaborator

Could we please flag this as a known issue, by tweaking release notes for cabal-install versions as suggested on #9955? Why would we partially revert for the 3.12 branch when it won't exhibit the problem of #9901, will it?

The serendipitous fix for #9901, bd0d321 (not a workaround), has already been backported to 3.12.

@ulysses4ever
Copy link
Collaborator

@Mikolaj let me see if I understand the situation correctly.

#9901 describes a dev experience regression associated with this patch (you can't run cabal-install from subdirs when developing Cabal-the-project). My reasoning for backporting this patch anyway was that developers normally develop against the master branch. So, I don't expect this patch to bring much problem for an average dev when backported to the 3.12 branch. Am I missing something?

Generally, I have three reasons to do a backport:

  1. bugfix (doesn't apply here)
  2. documentation improvement (doesn't apply here)
  3. refactoring that doesn't change the behavior or API but if not backported will make future backports more painful (arguably, applies here).

So, p. 3 was my reason for backporting this. If you still think we should reconsider, I'm open to suggestions.


@Mikolaj @philderbeast
A point of order. I must confess I don't follow the whole discussion in #9901 and how #9955 applies here. So, if you have suggestions that could improve the situation with this backport, please, spell them out, or reference particular comments of the tickets, or simply quote. Thanks!

@Mikolaj
Copy link
Member

Mikolaj commented May 14, 2024

Could we please flag this as a known issue, by tweaking release notes for cabal-install versions as suggested on #9955?

IMHO, this is a much better idea for a resolution of #9901 than doing nothing, but I think we should resolve it in #9901, not here nor in #9955.

Why would we partially revert for the 3.12 branch when it won't exhibit the problem of #9901, will it?

I think it can exhibit the problem. A dev backporting a nontrivial PR to 3.12 branch and needing to run the problematic tests there will encounter this problem and, possibly, assume it comes from the backported code.

The serendipitous fix for #9901, bd0d321 (not a workaround), has already been backported to 3.12.

Wow, that would be great. Why did I miss it? Why is there no mention of #9578 fixing #9901 in #9901? @philderbeast: could you explain it there? If that's not a misunderstanding, all my worries about this current backport are ill-founded and we don't need to waste time discussing that.

@Mikolaj
Copy link
Member

Mikolaj commented May 15, 2024

Why did I miss it?

Oh, I see, @philderbeast, you probably mean the scenario when somebody using cabal-install compiled from branch 3.12 is developing cabal (e.g, on that branch). That's not the scenario from #9901, which assumes devs are using recent versions of cabal from ghcup, etc. If they develop cabal on branch 3.12, this backport makes them hit #9901.

I don't expect this patch to bring much problem

@ulysses4ever: we have an in the wild report on #9901 from a developer that followed common dev practices (as far as I understand them; @philderbeast makes me aware that an alternative common dev practice is to always use tools recently compiled from the repo one is developing, e.g., to dogfood them) and got some of his time wasted by diagnosing this problem. Devs' time is precious, so I'm not comfortable incurring even the lower risk associated on hacking on branch 3.12 (e.g., to iron out backports). Unless there's a convincing benefit and 3 is reasonable, but if we haven't considered the trade-off consciously, that's precisely what I'm proposing to do here.

@ulysses4ever
Copy link
Collaborator

@Mikolaj let me revert it then.

@Mikolaj
Copy link
Member

Mikolaj commented May 15, 2024

@ulysses4ever: if we get (relative) consensus on #9901 (comment), then I think it may be enough to port the (to be written) recommendation in CONTRIBUTING.md to branch 3.12 [edit: I mean, instead of reverting]. Thoughts?

@philderbeast
Copy link
Collaborator

the (to be written) recommendation in CONTRIBUTING.md

I've added #10013 for the recommendation.

@ulysses4ever
Copy link
Collaborator

@Mikolaj since #9901 was closed and a patch to CONTRIBUTING.md will land real soon, shall we deem this resolved and move on?

@Mikolaj
Copy link
Member

Mikolaj commented May 15, 2024

@ulysses4ever: #9901 is not closed, but I haven't heard objections, so I hope the recommendation and a point in 3.12 release notes is an acceptable closure to the story. Let's move on. :)

@ulysses4ever
Copy link
Collaborator

Shoot! I was looking at #9923, my bad.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport squash+merge me Tell Mergify Bot to squash-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants