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 #9565

Merged
merged 6 commits into from
Mar 26, 2024
Merged

Use package groups #9565

merged 6 commits into from
Mar 26, 2024

Conversation

philderbeast
Copy link
Collaborator

@philderbeast philderbeast commented Dec 26, 2023

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:

@philderbeast
Copy link
Collaborator Author

I've left all the working commits there for now so the steps can be reviewed but I'm happy to squash these too when ready.

@philderbeast
Copy link
Collaborator Author

Do we use "libonly" to mean tests?

-- project-cabal/pkgs/tests.config
packages: Cabal-QuickCheck
packages: Cabal-described
packages: Cabal-tests
packages: Cabal-tree-diff
packages: cabal-testsuite

@Mikolaj
Copy link
Member

Mikolaj commented Dec 27, 2023

Do we use "libonly" to mean tests?

That rings a bell, though you haven't linked to a particular place in code. I think it's a flag that causes tests to test only Cabal-the-library or something like that. CI uses it, IIRC, in one of its phases.

@Mikolaj
Copy link
Member

Mikolaj commented Dec 27, 2023

BTW, I think the separate commits help a lot to understand what is going on, so I'd leave them be.

I hope all the project files are actually used and not dead code themselves...

@philderbeast
Copy link
Collaborator Author

@Mikolaj I saw "libonly" in the names of two projects:

  • cabal.project.libonly
  • cabal.project.validate.libonly

The packages in each look to be for testing:

-- project-cabal/pkgs/tests.config
packages:
    Cabal-QuickCheck
  , Cabal-described
  , Cabal-tests
  , Cabal-tree-diff
  , cabal-testsuite

@Mikolaj
Copy link
Member

Mikolaj commented Dec 27, 2023

Oh, right, and the project files only contain packages relevant to Cabal-the-library, so it figures.

@ulysses4ever
Copy link
Collaborator

I hope all the project files are actually used and not dead code themselves...

I'm afraid many of them are dead wood. Most used are cabal.project and cabal.project.validate — locally and in CI respectively. The rest should be grepped and if nothing comes out could be removed imo. One exception is cabal.project.release, which should be checked against the wiki page on making a release (if not mentioned there, could be removed I think).

@Kleidukos
Copy link
Member

I used cabal.project.release to produce FreeBSD bindists for the 3.10.2 series. I hope it was the correct choice. :')

@ulysses4ever
Copy link
Collaborator

Good! Then this is the third one we know is in use. The rest is still under a big question mark.

@Mikolaj
Copy link
Member

Mikolaj commented Dec 27, 2023

Right, some may be used by github CI and others by gitlab CI (the gitlab CI config is in our repo, too).

@philderbeast
Copy link
Collaborator Author

The only project not referenced is cabal.project.coverage and this was the one that wouldn't parse. I'll delete it.

cabal.project.buildinfo

$ grep -rwn --exclude-dir=".git" . -e 'cabal.project.buildinfo'
./Makefile:63:	cabal build --builddir=dist-newstyle-bi --project-file=cabal.project.buildinfo buildinfo-reference-generator

cabal.project.coverage

$ grep -rwn --exclude-dir=".git" . -e 'cabal.project.coverage'

cabal.project.doctest

$ grep -rwn --exclude-dir=".git" . -e 'cabal.project.doctest'
./Makefile:88:	cabal repl --with-ghc=doctest --build-depends=QuickCheck --build-depends=template-haskell --repl-options="-w" --project-file="cabal.project.doctest" Cabal-syntax
./Makefile:89:	cabal repl --with-ghc=doctest --build-depends=QuickCheck --build-depends=template-haskell --repl-options="-w" --project-file="cabal.project.doctest" Cabal

cabal.project.libonly

$ grep -rwn --exclude-dir=".git" . -e 'cabal.project.libonly'
./Makefile:82:# file from cabal/doctest. This is easy: we just select a project file with no allow-newer (e.g. cabal.project.libonly).

cabal.project.meta

$ grep -rwn --exclude-dir=".git" . -e 'cabal.project.meta'
./Makefile:42:	cabal run --builddir=dist-newstyle-meta --project-file=cabal.project.meta gen-spdx -- templates/SPDX.LicenseId.template.hs $(SPDX_LICENSE_VERSIONS:%=license-list-data/licenses-%.json) $(SPDX_LICENSE_HS)
./Makefile:45:	cabal run --builddir=dist-newstyle-meta --project-file=cabal.project.meta gen-spdx-exc -- templates/SPDX.LicenseExceptionId.template.hs $(SPDX_LICENSE_VERSIONS:%=license-list-data/exceptions-%.json) $(SPDX_EXCEPTION_HS)
./Makefile:55:	cabal run --builddir=dist-newstyle-meta --project-file=cabal.project.meta gen-cabal-macros -- $< $@
./Makefile:58:	cabal run --builddir=dist-newstyle-meta --project-file=cabal.project.meta gen-paths-module -- $< $@
./Makefile:68:	find Cabal-syntax/src Cabal/src cabal-install/src -type f -name '*.hs' | xargs cabal run --builddir=dist-newstyle-meta --project-file=cabal.project.meta analyse-imports --
./Makefile:156:	cabal run --builddir=dist-newstyle-meta --project-file=cabal.project.meta gen-validate-dockerfile -- $* $< $@

cabal.project.release

$ grep -rwn --exclude-dir=".git" . -e 'cabal.project.release'
./README.md:66:    cabal install --project-file=cabal.project.release cabal-install
./Makefile:194:	cabal build --project=cabal.project.release --with-compiler=ghc-$* --dry-run cabal-install:exe:cabal
./.gitlab/ci.sh:57:    echo 'constraints: lukko -ofd-locking' >> cabal.project.release.local
./.gitlab/ci.sh:64:    --project-file=cabal.project.release
./CONTRIBUTING.md:15:cabal build --project-file=cabal.project.release cabal

cabal.project.validate

$ grep -rwn --exclude-dir=".git" . -e 'cabal.project.validate(?!\.libonly)' --perl-regexp
./cabal-testsuite/README.md:40:cabal repl --with-ghc=doctest --build-depends=QuickCheck --build-depends=template-haskell --repl-options="-w" --project-file="cabal.project.validate" Cabal-syntax
./validate.sh:303:    PROJECTFILE=cabal.project.validate
./project-cabal/ghc-latest.config:6:--   - cabal.project.validate (Cabal CI),
./.github/workflows/validate.yml:132:          CABAL_EXEC=$(cabal list-bin --builddir=dist-newstyle-validate-ghc-${{ matrix.ghc }} --project-file=cabal.project.validate cabal-install:exe:cabal)

cabal.project.validate.libonly

$ grep -rwn --exclude-dir=".git" . -e 'cabal.project.validate.libonly'
./validate.sh:301:    PROJECTFILE=cabal.project.validate.libonly

cabal.project.weeder

$ grep -rwn --exclude-dir=".git" . -e 'cabal.project.weeder'
./Makefile:182:	cabal build all --project-file=cabal.project.weeder

@ulysses4ever
Copy link
Collaborator

Thank you for the analysis. I expected them to show up in the Makefile. The issue is: I don’t think the Makefile itself is widely used. Just look at all the Dockerfile stuff in the Makefile — it’s horrible. It would be nice to clean up the Makefile too.

Let’s look closer at your results:

  • I’m pretty sure we don’t use weeded yet (it’d be nice to do it, and of course there is a ticket for it, but we currently don’t), so that one could go.
  • cabal.project.libonly looks like a false positive that could go (it’s mentioned in a comment only)
  • cabal.project.buildinfo is dubious
  • *.meta is mentioned many times in the Makefile but I don’t know that anyone uses it except maybe CI: there is a Meta job in it; someone needs to double check whether CI calls the targets of the Makefile that has to do with this project file.

More radically, I propose to cut all the project files and Makefile targets that

  • neither exercised by our CI (GitHub+gitlab)
  • nor referenced from the wiki page on Making a release.
    Note that to check this condition you have to build a transitive closure: CI can mention a Makefile target that mentions a project file, and so you should preserve the project file…

Putting it positively, we need to make sure that CI uses the Makefile as much as possible, but everything that is not used in CI should be assumed to bit rot and removed. I think there are rare exceptions from this rule that sometimes come from @andreasabel who afair use some non-CI targets in the Makefile. Maybe from @Kleidukos too.

@Kleidukos
Copy link
Member

Yeah I use the makefile for styling.

@ulysses4ever
Copy link
Collaborator

Oh, right, the style (and related) target is the recommended way to do it. So, I patch the rule above:

More radically, I propose to cut all the project files and Makefile targets that

  • neither exercised by our CI (GitHub+gitlab),
  • nor referenced from the wiki page on Making a release,
  • nor mentioned in CONTRIBUTING.md

@philderbeast
Copy link
Collaborator Author

I tried make weeder on the master branch at 52d1fb6:

$ make weeder
cabal build all --project-file=cabal.project.weeder
Resolving dependencies...
Error: [Cabal-7107]
Could not resolve dependencies:
[__0] trying: cabal-install-3.11.0.0 (user goal)
[__1] next goal: cabal-install-solver (dependency of cabal-install)
[__1] rejecting: cabal-install-solver-3.10.2.1 (conflict: cabal-install => cabal-install-solver^>=3.11)
[__1] skipping: cabal-install-solver-3.10.1.0, cabal-install-solver-3.8.1.0 (has the same characteristics that caused the previous version to fail: excluded by constraint '^>=3.11' from 'cabal-install')
[__1] fail (backjumping, conflict set: cabal-install, cabal-install-solver)
After searching the rest of the dependency tree exhaustively, these were the goals I've had most trouble fulfilling: cabal-install, cabal-install-solver

make: *** [Makefile:182: weeder] Error 1

The cabal-testsuite .hie files will be a problem for weeder and with weeder >= 2.6 the configuration has changed to TOML. I'll go ahead and remove weeder on this pull request. It can be added back with #9121.

@philderbeast
Copy link
Collaborator Author

philderbeast commented Dec 28, 2023

I've updated the Makefile recipe for doc/buildinfo-fields-reference.rst instead of using a phony target. Some fixes will be needed to get the users guide to build.

/home/runner/work/cabal/cabal/doc/buildinfo-fields-reference.rst:516: WARNING: cabal:pkg-field reference target not found: virtual-modules

Note

Changing the reference to library:virtual-modules fixed this problem.

@Mikolaj
Copy link
Member

Mikolaj commented Mar 26, 2024

@mergify dequeue

Copy link
Contributor

mergify bot commented Mar 26, 2024

dequeue

☑️ The pull request is not queued

@Mikolaj
Copy link
Member

Mikolaj commented Mar 26, 2024

@mergify queue

Copy link
Contributor

mergify bot commented Mar 26, 2024

queue

🛑 The pull request has been merged manually

The pull request has been merged manually at 7e085fa

@Mikolaj
Copy link
Member

Mikolaj commented Mar 26, 2024

I think mergify lost track of the approved-reviews-by number.

Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing.

Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing 2.

@Mikolaj
Copy link
Member

Mikolaj commented Mar 26, 2024

@philderbeast: could you kindly rebase manually and then manually merge with no-fast-forward (e.g., via the "merge pull request" button)? Thank you!

- 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.
- 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
- 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)
* 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
@philderbeast philderbeast merged commit 7e085fa into haskell:master Mar 26, 2024
55 checks passed
@philderbeast
Copy link
Collaborator Author

@Mikolaj thanks very much for debugging mergify. I've made the merge.

@Mikolaj
Copy link
Member

Mikolaj commented Mar 26, 2024

Well done! :)

@philderbeast
Copy link
Collaborator Author

I validated master locally to be sure after the merge and it is all good (after the 15 min wait).

$ ./validate.sh
...
!!! Validation took 908 seconds.

geekosaur added a commit to geekosaur/cabal that referenced this pull request Apr 20, 2024
cabal-install 3.10 mis-handles relative cabal.project imports, as
added in haskell#9565. As a hack, try
using a magic symlink "@" in every tracked directory that links to
the top directory, and use it in the import paths.

This stands a certain chance of failing on Windows.
@ulysses4ever
Copy link
Collaborator

@mergify backport 3.12

Copy link
Contributor

mergify bot commented May 13, 2024

backport 3.12

✅ Backports have been created

@mergify mergify bot mentioned this pull request May 13, 2024
1 task
mergify bot added a commit that referenced this pull request May 13, 2024
* Import packages from project-cabal/pkgs

- 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

* Build all local packages with -Werror

- 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)

* Add an intercepting cabal-testsuite/cabal.project

- 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 dangling doctest TODO left over from #9572

(cherry picked from commit 619befd)

* Remove allow-newer exception for cryptohash-sha256:base

(cherry picked from commit ed5d404)

* Remove projects; weeder, doctest & libonly

* 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)

* Update cabal.project.release

---------

Co-authored-by: Phil de Joux <[email protected]>
Co-authored-by: Hécate <[email protected]>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge+no rebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Could we simplify our use of projects?
8 participants