-
Notifications
You must be signed in to change notification settings - Fork 321
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
jokesper
wants to merge
19
commits into
kmonad:master
Choose a base branch
from
jokesper:refactor
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jokesper
force-pushed
the
refactor
branch
2 times, most recently
from
September 28, 2024 20:00
95db01d
to
df59537
Compare
jokesper
force-pushed
the
refactor
branch
2 times, most recently
from
October 10, 2024 19:15
3819c00
to
0914bfe
Compare
jokesper
changed the title
Refactor with minor improvements
Refactor with ci related improvements
Oct 16, 2024
jokesper
force-pushed
the
refactor
branch
4 times, most recently
from
October 18, 2024 23:35
1f02964
to
f46c71f
Compare
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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).
(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 islocal
.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
DeriveAnyClass
(also don't add ", commit" suffix, if we don't know it**)
Type
s submodules into Model, since they fit there more.rio
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