-
Notifications
You must be signed in to change notification settings - Fork 417
Modernise and fix the nix flake #19185
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
base: develop
Are you sure you want to change the base?
Conversation
Signed-off-by: Rory& <[email protected]>
Signed-off-by: Rory& <[email protected]>
Signed-off-by: Rory& <[email protected]>
Signed-off-by: Rory& <[email protected]>
|
Identified a missing piece: i'm unable to run the linter, not sure what all i'm missing or how to get that working, since the cargo thingy seems to throw a fit when i run lint.sh if i |
anoadragon453
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking the time to do this! A few notes below.
| # NOTE: We currently need to set the Rust version unnecessarily high | ||
| # in order to work around https://github.com/matrix-org/synapse/issues/15939 | ||
| (rust-bin.stable."1.82.0".default.override { | ||
| (rust-bin.stable."1.88.0".default.override { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be at least 1.89, so that we can compile ruff: https://github.com/astral-sh/ruff/blob/1a86e1347200c1db319b9f9960799c1b192bb3fd/Cargo.toml#L8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: is there any risk in putting the version too high, or should it be kept at the minimum to ensure compatibility?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No risk. Typically the higher the better with Rust in the dev environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might aswell bump it to 1.91.1 then, I suppose
| # Workaround to make cargo fetch git repositories with the git CLI | ||
| env.CARGO_NET_GIT_FETCH_WITH_CLI = "true"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be ideal to justify why this is needed, in case it has unintended side-effects.
| # Ensure the venv is activated explicitly, in order not to confuse poetry | ||
| . .venv/bin/activate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by "confuse poetry"? What happens if this isn't here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd been doing this manually for ages - if I skip that step, for some reason, any poetry command causes the .venv to get cleared, causing synapse to fail to start due to missing dependencies.
I could probably clarify that in the comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be useful to do so, yeah.
| debian-devscripts # (`dch` for manipulating the Debian changelog) | ||
| libnotify # (the release script uses `notify-send` to tell you when CI jobs are done) | ||
|
|
||
| # For building psychopg2 from source, if needed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| # For building psychopg2 from source, if needed | |
| # For building psycopg2 from source, if needed |
lol
| @@ -0,0 +1 @@ | |||
| flake.lock -diff | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| flake.lock -diff | |
| # flake.lock is only meant to be read by machines. | |
| flake.lock -diff |
| @@ -0,0 +1,2 @@ | |||
| #! /usr/bin/env sh | |||
| poetry run trial -j`nproc` tests | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if such a short command really justifies a script?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I keep forgetting and having to hunt it down in the documentation, so it's just there as an aid.
Can be dropped if you want :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeahhh, this just doesn't quite sit right with me.
If you like, you could add a single-line script to the nix flake instead? https://devenv.sh/scripts/
Summarised changes:
pg_configis available if we need to build psychopg2 from source for some reasonPull Request Checklist
EventStoretoEventWorkerStore.".code blocks.