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

Refactor with ci related improvements #896

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

jokesper
Copy link
Contributor

@jokesper jokesper commented Sep 27, 2024

Hello,

if anyone wants to review this I suggest, looking through the commits individually (and taking a look at the commit body message if available).
Furthermore I would also collect the questions about why things have been done a certain way till the end,
since sometimes, this is only really useful in combination with later commits.

I don't expect this to get merged (any time soon), it's just my personal effort to clean up the code base somewhat.

This was originally started of while taking a look around some code and I found some areas which could be somewhat improved to aid readability and maintainability.
Then the following things got done (some should probably live in a separate PR, as they fix actual relevant issues).

  1. Reduction in binary size (-split-sections) which got removed in Update compiler and build setup in the dockerfile #880
    (What happened between 0.4.2 and 0.4.3 that the binary size increased from 3.5MiB to 13.8MiB?).
    After bisecting I found out that most of the increase happened in Update compiler and build setup in the dockerfile #880 even if we include -split-sections.
    This is because before we applied it to every package and after it depends on apply-ghc-options which is local.
    Meaning all flags passed to the first stack build don't matter.
    I used the same mechanism to differentiate between flags meant for both and flags meant only for kmonad
    and was able to merge those build commands into one to not have to duplicate the editing of stack.yaml
  2. Reenable ci for windows (We had this the whole time?!).
  3. Added a dockerfile to build a windows release. (I hate docker, wine and msys2) (Could be used to fix No windows binaries for the latest release? #767 / Please provide precompiled binaries for windows again #818)
  4. Interesting C code, which got improved (slightly) to remove warnings in Windows CI.
  5. Added keycodes from windows and merge tables #894 would have fit perfectly here.
  6. Syntax change to be consistent and have clearer diffs when changing CI.
  7. More windows CI + some code cleanup.
  8. Why did we ever ignore warnings?
  9. Make use of a Custom Prelude which is a Prelude and doesn't have to be imported.
  10. Why do we have a directory with only one file in it. (app/)
  11. We don't need to always specify DeriveAnyClass
  12. KMonad.Args.TH can be made vastly more readable
    (also don't add ", commit" suffix, if we don't know it**)
  13. Deduplicate Config data types, using type families, helps with name collisions. also includes some fixes while reimplementing stuff. (Most of the changes)
  14. Moved some Types submodules into Model, since they fit there more.
  15. Reduced usages of partial functions
  16. Added warnings recommended by rio
  17. Made the parsing code more robust and unified. Also slightly improves error messages.

t.l.d.r.: No major changes just some minor* ones which bothered me while working on the code base.

If this ever gets reviewed (sorry), I apologize for the large diff and would probably recommend going at it per commit and not in total.

* and some CI improvements (which caused this refactoring spree)
** when getting the source from the release page or cabal get

@jokesper jokesper force-pushed the refactor branch 2 times, most recently from 95db01d to df59537 Compare September 28, 2024 20:00
@jokesper jokesper force-pushed the refactor branch 2 times, most recently from 3819c00 to 0914bfe Compare October 10, 2024 19:15
@jokesper jokesper changed the title Refactor with minor improvements Refactor with ci related improvements Oct 16, 2024
@jokesper jokesper force-pushed the refactor branch 4 times, most recently from 1f02964 to f46c71f Compare October 18, 2024 23:35
We can reduce the resulting binary size by passing `-split-section`
to ghc. We do this under `$everything` in the config file, since
otherwise only the local package would be affected not all dependencies.
The built time is as far as I can tell only slightly affected and
we reduce the size from ~15MiB to ~4MiB.

We also merge build of dependencies and kmonad and use `--ghc-options`
to specify which options we want only for kmonad.
With this, we don't have to duplicate editing `stack.yaml`.
This means less we cannot reuse build artifacts from dependencies.
Since the CI does not do any caching, this only changes for local
builds, in which case you wouldn't need a dockerfile and could simply
do a `stack build`.
We don't use stack, since MSYS2 is broken.
Therefore we get cabal to use stackage.

`--split-sections` does not seem to do anything,
but stripping does do a lot.
This only affects local builds, since we don't do any caching
in the CI (yet).
As far as I could tell, it would have been compiled to a zero.
I think changing it to non zero to block other event lowlevel handlers
is the right decision and may fix some bugs, though I am not on Windows
and am unable to test any of this.
When we change which stack resolvers we use commonly we would add a new
ones and remove old ones.

This means we delete from the top and add to the bottom.

Deleting from the top is a clean diff with trailing comma,
while adding to the bottom is a clean diff with leading comma.

With our current leading comma approach we would have to always change
two lines at the beginning.
`-Wunused-imports`

- KMonad.Prelude:
  - GHC.Conc (orElse) is exported by RIO via UnliftIO.STM
  - Removed KMonad.Prelude.Definitions as it is empty
- KMonad.Keyboard.IO.Linux.UinputSink:
  - UnliftIO.Async (async) is exported by KMonad.Prelude via RIO
- *:
  - KMonad.Keyboard.IO is not needed and should not be needed there

`-Wname-shadowing`

Mark with tick except for KMonad.Args.Cmd (cmd) in KMonad.Args
since it doesn't add clarity

`-Wx-partial`

- KMonad.Gesture:
  - Use view patterns with `S.lookupMin` instead of `S.null`
    with `head . S.elems` on case failure.
    Cases had to be switched, since old GHC versions don't detect
    exhaustive pattern matching when done by listing all constructors
    in view patterns.
    Module   | Total | Lib | Test
-------------+-------+-----+------
 KMonad.Util |    10 |  10 |    0
 RIO.Char    |     3 |   2 |    1
 RIO.Text*   |     1 |   1 |    0
-------------+-------+-----+------
             |    14 |  13 |    1

* Duplicate import which was qualified and could therefore evade GHC
  warnings
Since our `main` function is just a reexport we don't need an extra directory
or `base` as a dependency.
Document all language extensions we are using in `kmonad.cabal`
(populate `other-extensions`) and enable `DeriveAnyClass` by default.
The extra `transformers` dependency was already implied by `mtl`,
so why not use it?
This commit unifies the following data structures with the help of type families
- 'DefSetting' -> 'Parsing'
- 'CfgToken'   -> 'Token'
- 'JCfg'       -> 'Joining'
- 'Cmd'        -> 'Cmd'
- 'AppCfg'     -> 'App'
- 'AppEnv'     -> 'Env'

Further changes:
- Config values are merged via a Monoid instance
  => 'joinConfig' can use pattern matching for duplication checking
- 'output' setting when missing was wrongly reported as 'input'
- Disallow compose sequences such as 'ä' as a compose key
- Print the reason why a compose key was invalid
- You can specify multiple 'defcfg' which get merged
- You can now disable 'fallthrough' and 'allow-cmd' via the command line.
- Fixed some 'lexeme' stuff
  - Empty 'defcfg' is a parse error not a join error
  - 'lexeme (lexeme ...)'
- 'DefSetting' equality instance is now a normal instance, since we no longer
  use it when merging a 'CmdL' and 'PCfg' and only need it for tests.
This should not break any existing configs, and improve the error messages somewhat.
The messages are still not perfect, as we don't do a separate tokenizing step
(which is very hard, since `"` is either the start of a string or a button,
depending of the context).
This should also reduce the duplicate `lexeme`s sometimes found, and make the code
less confusing in regards to when to use them.
Furthermore it is more robust, i.e. less order dependence of parsing
branches, and less back tracking, as `try` is only applied when we
really need it.
We let the primitives do `lexeme`, and therefore (mostly) don't need any in other functions.
Furthermore we use `delimited` instead of `lexeme`, since this ensures we don't put
keywords next to each other (see the downsides below).
Technically sometimes a `lexeme` is missing if the token doesn't parse anything.
This can happen due to an empty `many` or an `option`. In this case the `lexeme`
is should be covered by the previous token.

I have tested this against `tutorial.kbd`, my personal config and `kmonad-contrib`.
There were only errors in:
- nharward's config, since no `defcfg` blocks are defined
- `david-janssen/neo-test.kbd`, since this uses unknown compose sequences and has been
  broken since at least 0.4.0.

The downside is, that my favourite config broke*:
```
(defcfgcmp-seqS-+~input(device-file"")cmp-seqP0output(uinput-sink""))
(defsrc-)
(deflayer(;; (around-"))
```

* was technically broken before, due to the config joining rewrite
  fixing duplicate `cmp-seq`s not being an error.
@jokesper jokesper mentioned this pull request Oct 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No windows binaries for the latest release?
1 participant