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

Add alex to build-tool-depends remove separate code gen #8980

Merged
merged 6 commits into from Aug 10, 2023

Conversation

andreabedini
Copy link
Collaborator

In Distribution.Fields.Lexer we have this note.

-- [Note: bootstrapping parsec parser]
--
-- We manually produce the `Lexer.hs` file from `boot/Lexer.x` (make lexer)
-- because bootstrapping cabal-install would be otherwise tricky.
-- Alex is (atm) tricky package to build, cabal-install has some magic
-- to move bundled generated files in place, so rather we don't depend
-- on it before we can build it ourselves.
-- Therefore there is one thing less to worry in bootstrap.sh, which is a win.
--
-- See also https://github.com/haskell/cabal/issues/4633

The issue linked in the note mentions that alex has build-type: Simple since Feb 2018. Moreover, bootstrap.sh, has been replaced by a Python script that seems to be more powerful.

I haven't actually tried to bootstrap yet, so opening this as a draft.


Please include the following checklist in your PR:

Bonus points for added automated tests!

@andreabedini
Copy link
Collaborator Author

The most annoying part in testing this is that cabal-bootstrap-gen doesn't know about xdg-dirs :-/

cp dist-newstyle/cache/plan.json bootstrap/linux-8.10.7.plan.json
cd bootstrap && cabal v2-run -v0 cabal-bootstrap-gen -- linux-8.10.7.plan.json \
	| python3 -m json.tool > linux-8.10.7.json
cabal-bootstrap-gen: /home/andrea/.cabal/config: withBinaryFile: does not exist (No such file or directory)
Expecting value: line 1 column 1 (char 0)
make: *** [Makefile:195: bootstrap-json-8.10.7] Error 1

@andreabedini
Copy link
Collaborator Author

andreabedini commented May 30, 2023

I am afraid this need a fair bit of rework. It's not only bootstrap.py that needs to change but also cabal-bootstrap-gen. Its view of the plan is too simplistic and does not understand per-component builds.

I notice that cabal-plan already gives us a good "execution plan":

❯ cabal-plan topo --plan-json linux-8.10.7.plan.json --show-flags
cabal-install-3.11.0.0 exe:cabal +lukko +native-dns
cabal-install-3.11.0.0 +lukko +native-dns
safe-exceptions-0.1.7.3
resolv-0.1.2.0
regex-posix-0.96.0.1 -_regex-posix-clib
regex-base-0.94.0.2
hackage-security-0.6.2.3 +base48 +cabal-syntax +lukko -mtl21 -old-directory +use-network-uri
zlib-0.6.3.0 -bundled-c-zlib -non-blocking-ffi -pkg-config

This does include the components to build in the correct order and also exe-depends like alex.

Edit: cabal-bootstrap-gen already uses cabal-plan, there's hope!

@andreabedini
Copy link
Collaborator Author

andreabedini commented Jun 1, 2023

This seems to work now. I was surprised we have no alex-options in .cabal.

I could pass --latin1 as an in-file annotation but I am not sure if I can do the same with --ghc.

The build script now performs a per-component build which works in this case but won't work in full generality (because it doesn't respects qualified goals and flattens everything different goals in a single plan).

@andreabedini andreabedini marked this pull request as ready for review June 2, 2023 03:26
@andreabedini
Copy link
Collaborator Author

D:\a\cabal\cabal\cabal-testsuite\PackageTests\MultiRepl\EnabledSucc\cabal.dist\work\.\dist\multi-out-5488\paths\pkg-b-0-inplace: removeDirectoryRecursive:removeContentsRecursive:removePathRecursive:removeContentsRecursive:removePathRecursive:DeleteFile "\\\\?\\D:\\a\\cabal\\cabal\\cabal-testsuite\\PackageTests\\MultiRepl\\EnabledSucc\\cabal.dist\\work\\dist\\multi-out-5488\\paths\\pkg-b-0-inplace": permission denied (The process cannot access the file because it is being used by another process.)

CI has some hiccups. I'll kick the tire again.

@ulysses4ever
Copy link
Collaborator

I'm sorry i need some time to look into it: I'm not terribly familiar with how we use alex and bootstrapping in general. I'd be happy to review, but I'll need to find a chunk of time to first educate myself in these matters a bit.

@andreabedini
Copy link
Collaborator Author

@ulysses4ever No worries. Nothing urgent about this ☺️

@ulysses4ever
Copy link
Collaborator

@andreasabel we're lacking experience here. Could you maybe provide a review for this alex-related PR?

Copy link
Member

@andreasabel andreasabel left a comment

Choose a reason for hiding this comment

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

From afar, LGTM, provided it works. Thanks!
The --ghc flag to alex should be supplied automatically by cabal, so I think we need not worry about that.
There are unnecessary diffs in the linux-*.json file due to reorderings. Maybe the generator for these files could be fixed so that random reorderings do not happen.

{
"package": "binary",
"version": "0.8.8.0"
},
Copy link
Member

Choose a reason for hiding this comment

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

Minor comment: Would it make sense to order this list alphabetically by package name so that diffs are more stable.
In this case, the diff is just a reordering, this could/should be avoided.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a good idea 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, no 😂

The lines you highlight are from the builtin section (aka pre-installed packages). Those don't really need to follow a specific order, and they are used only for validation in bootstrap.py. But the other list of packages has to be in topological order otherwise we will find missing dependencies. This is why it's a list and not a dictionary.

I can turn the builtin section into a dict, but I'd avoid changing the rest.

Makefile Show resolved Hide resolved
@andreabedini andreabedini force-pushed the build-tool-depends-alex branch 2 times, most recently from 2a951e4 to c48ccb2 Compare July 12, 2023 08:47
Copy link
Member

@andreasabel andreasabel left a comment

Choose a reason for hiding this comment

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

From the alex side, this LGTM.
(I am not qualified on the bootstrap process here, so I cannot review this part sensibly.)

@andreabedini
Copy link
Collaborator Author

Do we know who is using this script? Can we get their input?

@andreabedini
Copy link
Collaborator Author

andreabedini commented Jul 12, 2023

@arrowd @maralorn Do you use this bootstrap script?

@arrowd
Copy link
Collaborator

arrowd commented Jul 12, 2023

Yes, I do. The change seems fine to me, but I only had a quick look.

@maralorn
Copy link
Contributor

I have no clue. Maybe @sternenseemann knows better.
The only thing I can say is that we in principle build happy and alex for bootstraping ghc, so they should be available when building Cabal as well.

@sternenseemann
Copy link

We don't use cabal-install anywhere in package builds (except for ghcjs which is only built when cabal-install is easily attainable), so cabal-install is nothing we need to bootstrap at all, thanks to Setup.hs and GHC taking care of bootstrapping Cabal.

@sternenseemann
Copy link

Alpine Linux definitely uses it, so maybe @nmeum could take a look?

andreabedini and others added 4 commits July 21, 2023 18:25
- Pass latin-1 encoding is passed as input pragma
- Cabal hard-codes -g for alex so we don't need to worry about --ghc
- Rework bootstrap, adding per-component building
- Update bootstrap plans
@andreabedini
Copy link
Collaborator Author

Can I haz another review?

@andreabedini
Copy link
Collaborator Author

That was quick @Kleidukos! <3

@andreabedini andreabedini added squash+merge me Tell Mergify Bot to squash-merge and removed attention: needs-review labels Aug 8, 2023
@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Aug 10, 2023
@mergify mergify bot merged commit 0ab5571 into haskell:master Aug 10, 2023
44 checks passed
@andreabedini andreabedini deleted the build-tool-depends-alex branch August 10, 2023 08:32
andreabedini added a commit to andreabedini/cabal that referenced this pull request Aug 23, 2023
Mikolaj pushed a commit to andreabedini/cabal that referenced this pull request Sep 6, 2023
mergify bot added a commit that referenced this pull request Sep 7, 2023
fix(Cabal-syntax): Remove left-over file after #8980
@Bodigrim
Copy link
Collaborator

Bodigrim commented Sep 17, 2023

(Upd.: Whoops, sorry, Lexer.x was removed in #9198, not here. Moving the bulk of the comment there)

When evaluating changes related to bootstrapping, it might help to bump Cabal submodule in GHC source tree and estimate the impact. There are test-bootstrap jobs: if they pass fine, we are all good.

Bodigrim added a commit that referenced this pull request Sep 17, 2023
Bodigrim added a commit that referenced this pull request Sep 17, 2023
BinderDavid pushed a commit to BinderDavid/cabal that referenced this pull request Sep 19, 2023
andreasabel added a commit that referenced this pull request Mar 26, 2024
andreasabel added a commit that referenced this pull request Mar 26, 2024
Mikolaj pushed a commit that referenced this pull request Mar 29, 2024
andreasabel added a commit that referenced this pull request Apr 2, 2024
coot pushed a commit that referenced this pull request Apr 6, 2024
erikd pushed a commit to erikd/cabal that referenced this pull request Apr 22, 2024
mergify bot pushed a commit that referenced this pull request May 21, 2024
The `lexer` target was removed in
#8980

(cherry picked from commit e600087)
mergify bot added a commit that referenced this pull request May 23, 2024
…9845) (#10041)

* Fix #9815: switch quick-jobs CI to XDG

Fix #9815:
- Cache `~/.local/state/cabal` instead of `~/.cabal/store`
- `~/.local/bin` is used instead of `~/.cabal/bin` and is already in the PATH
  (verify this by calling `alex` after installing it)

As I am passing by:
- bump cache action to v4
- double-quote `$USER` to keep actionlint happy
- move `if` from shell-level to job-level
- allow newest `alex`

(cherry picked from commit e916cb5)

* CI quick-jobs: use preinstalled GHC and Cabal

(cherry picked from commit c209a82)

* Makefile: remove dead target 'lexer', use '.PHONY' systematically

The `lexer` target was removed in
#8980

(cherry picked from commit e600087)

* CI "Meta checks": correct cache key

(cherry picked from commit 56426e4)

* CI "Meta checks": print Haskell versions

(cherry picked from commit 9a311bd)

* CI "Doctest Cabal": daily refresh of cache

(cherry picked from commit ba6f6ff)

* CI "Check Field Syntax Reference": correct cache key

(cherry picked from commit 5949e3f)

* Update generated Cabal/src/Distribution/Simple/Build/Macros/Z.hs

Not sure why this was not up to date on master and still CI passed.
Maybe the content of this file is dependent on the GHC version we are
using to build the `get-cabal-macros` tool?

(cherry picked from commit 947860a)

* CI quick-jobs: entirely wipe ghcup directory rights workaround

(cherry picked from commit 5aa8afd)

* !fixup

---------

Co-authored-by: Andreas Abel <andreas.abel@ifi.lmu.de>
Co-authored-by: Artem Pelenitsyn <a.pelenitsyn@gmail.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 squash+merge me Tell Mergify Bot to squash-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants