fix CI

CobaltCause commented

2023-12-24 04:39:11 +00:00

(Migrated from gitlab.com)

I agree to release my code and all other changes of this MR under the Apache-2.0 license

- [X] I agree to release my code and all other changes of this MR under the Apache-2.0 license

CobaltCause commented

2023-12-24 04:43:29 +00:00

(Migrated from gitlab.com)

added 5 commits

ed69d43e - comment out heed backend; code doesn't compile
6d4d9ca5 - use constructor to avoid deprecation warnings
57efa4b7 - fix `cargo check` lints
5ce8c324 - fix `cargo clippy` lints
68ee04ab - fix `cargo doc` lints

Compare with previous version

added 5 commits <ul><li>ed69d43e - comment out heed backend; code doesn't compile</li><li>6d4d9ca5 - use constructor to avoid deprecation warnings</li><li>57efa4b7 - fix `cargo check` lints</li><li>5ce8c324 - fix `cargo clippy` lints</li><li>68ee04ab - fix `cargo doc` lints</li></ul> [Compare with previous version](/famedly/conduit/-/merge_requests/564/diffs?diff_id=880484151&start_sha=a3ae80ff7ee1f59be000f302da33d94721b2042a)

timokoesters commented

2023-12-24 15:56:04 +00:00

(Migrated from gitlab.com)

Can you revert this to the explicit version, I prefer that one.

timokoesters commented

2023-12-24 15:56:04 +00:00

(Migrated from gitlab.com)

Can we allow that by default, in a file like clippy.toml? I want to keep clippy out of the main codebase if that is possible.

timokoesters commented

2023-12-24 15:56:04 +00:00

(Migrated from gitlab.com)

I wonder if it's possible to write filter_map(|r| r.ok()?) or something like that to avoid the flatten. But this is fine.

I wonder if it's possible to write `filter_map(|r| r.ok()?)` or something like that to avoid the flatten. But this is fine.

timokoesters commented

2023-12-24 15:56:05 +00:00

(Migrated from gitlab.com)

Maybe it's easier to read by matching the len with 0, 1, and 2..

timokoesters commented

2023-12-24 15:56:05 +00:00

(Migrated from gitlab.com)

Weird hack, but okay

timokoesters commented

2023-12-24 15:56:06 +00:00

(Migrated from gitlab.com)

I think == Ok(true) is easier to read.

timokoesters commented

2023-12-24 15:56:06 +00:00

(Migrated from gitlab.com)

Thanks a lot for your work, here are some small ideas for improvements

M0dEx commented

2023-12-24 16:00:13 +00:00

(Migrated from gitlab.com)

            .flat_map(|state| {
                services()
                    .rooms
                    .state_accessor
                    .room_state_get(room_id, &state.0, &state.1)
            })

```suggestion:-7+0 .flat_map(|state| { services() .rooms .state_accessor .room_state_get(room_id, &state.0, &state.1) }) ```

M0dEx commented

2023-12-24 16:01:33 +00:00

(Migrated from gitlab.com)

            .flat_map(|member| {
                Ok::<_, Error>(
                    services()
                        .rooms
                        .state_accessor
                        .get_member(room_id, &member)?
                        .map(|memberevent| {
                            (
                                memberevent
                                    .displayname
                                    .unwrap_or_else(|| member.to_string()),
                                memberevent.avatar_url,
                            )
                        }),
                )
            })

```suggestion:-17+0 .flat_map(|member| { Ok::<_, Error>( services() .rooms .state_accessor .get_member(room_id, &member)? .map(|memberevent| { ( memberevent .displayname .unwrap_or_else(|| member.to_string()), memberevent.avatar_url, ) }), ) }) ```

M0dEx commented

2023-12-24 16:03:04 +00:00

(Migrated from gitlab.com)

Couldn't we do something like matches!(services()..., Ok(true))? The original statement might be more readable, though.

Couldn't we do something like `matches!(services()..., Ok(true))`? The original statement might be more readable, though.

M0dEx commented

2023-12-24 16:12:41 +00:00

(Migrated from gitlab.com)

If I understand this correctly, the Ok is there to tell rustc that the return type of the closure is Result, to allow for ?, right? Personally, I find it a bit ugly, but I can't figure out a cleaner way.

If I understand this correctly, the `Ok` is there to tell `rustc` that the return type of the closure is `Result`, to allow for `?`, right? Personally, I find it a bit ugly, but I can't figure out a cleaner way.

CobaltCause commented

2023-12-24 20:38:34 +00:00

(Migrated from gitlab.com)

The explicit version triggers a deprecation warning though?

CobaltCause commented

2023-12-24 20:38:34 +00:00

(Migrated from gitlab.com)

Can we allow that by default, in a file like clippy.toml?

I think it would be best to keep this lint enabled, it's just that I didn't feel comfortable refactoring all the existing violations.

I want to keep clippy out of the main codebase if that is possible.

Why?

> Can we allow that by default, in a file like clippy.toml? I think it would be best to keep this lint enabled, it's just that I didn't feel comfortable refactoring all the existing violations. > I want to keep clippy out of the main codebase if that is possible. Why?

CobaltCause commented

2023-12-24 20:38:35 +00:00

(Migrated from gitlab.com)

Yeah...

CobaltCause commented

2023-12-27 04:11:16 +00:00

(Migrated from gitlab.com)

changed this line in version 3 of the diff

changed this line in [version 3 of the diff](/famedly/conduit/-/merge_requests/564/diffs?diff_id=881338732&start_sha=68ee04ab3cd234930fd0221690256e56dad9a9ab#312d406453d072f966568e7e9199c628ba9d6882_1566_1565)

CobaltCause commented

2023-12-27 04:11:16 +00:00

(Migrated from gitlab.com)

changed this line in version 3 of the diff

changed this line in [version 3 of the diff](/famedly/conduit/-/merge_requests/564/diffs?diff_id=881338732&start_sha=68ee04ab3cd234930fd0221690256e56dad9a9ab#312d406453d072f966568e7e9199c628ba9d6882_1597_1591)

CobaltCause commented

2023-12-27 04:11:17 +00:00

(Migrated from gitlab.com)

changed this line in version 3 of the diff

changed this line in [version 3 of the diff](/famedly/conduit/-/merge_requests/564/diffs?diff_id=881338732&start_sha=68ee04ab3cd234930fd0221690256e56dad9a9ab#6d9d1569922804e999b3392464af9e8afe9b7371_487_487)

CobaltCause commented

2023-12-27 04:11:17 +00:00

(Migrated from gitlab.com)

changed this line in version 3 of the diff

changed this line in [version 3 of the diff](/famedly/conduit/-/merge_requests/564/diffs?diff_id=881338732&start_sha=68ee04ab3cd234930fd0221690256e56dad9a9ab#312d406453d072f966568e7e9199c628ba9d6882_1566_1565)

CobaltCause commented

2023-12-27 04:11:18 +00:00

(Migrated from gitlab.com)

changed this line in version 3 of the diff

changed this line in [version 3 of the diff](/famedly/conduit/-/merge_requests/564/diffs?diff_id=881338732&start_sha=68ee04ab3cd234930fd0221690256e56dad9a9ab#312d406453d072f966568e7e9199c628ba9d6882_1594_1589)

CobaltCause commented

2023-12-27 04:11:19 +00:00

(Migrated from gitlab.com)

added 9 commits

68ee04ab...ca621972 - 4 commits from branch famedly:next
550eecdc - comment out heed backend; code doesn't compile
306af324 - use constructor to avoid deprecation warnings
e9f50e63 - fix `cargo check` lints
07951c2b - fix `cargo clippy` lints
426ec25c - fix `cargo doc` lints

Compare with previous version

added 9 commits <ul><li>68ee04ab...ca621972 - 4 commits from branch <code>famedly:next</code></li><li>550eecdc - comment out heed backend; code doesn't compile</li><li>306af324 - use constructor to avoid deprecation warnings</li><li>e9f50e63 - fix `cargo check` lints</li><li>07951c2b - fix `cargo clippy` lints</li><li>426ec25c - fix `cargo doc` lints</li></ul> [Compare with previous version](/famedly/conduit/-/merge_requests/564/diffs?diff_id=881338732&start_sha=68ee04ab3cd234930fd0221690256e56dad9a9ab)

CobaltCause commented

2023-12-27 04:12:41 +00:00

(Migrated from gitlab.com)

If we want to return a Result from the closure (e.g. via ?) then all returned values must be of type Result (e.g. in Ok(...)). The alternative to wrapping the whole expression in Ok(...) is to bind an intermediate variable and then wrap that at the end instead.

If we want to return a `Result` from the closure (e.g. via `?`) then all returned values must be of type `Result` (e.g. in `Ok(...)`). The alternative to wrapping the whole expression in `Ok(...)` is to bind an intermediate variable and then wrap that at the end instead.

CobaltCause commented

2023-12-27 04:13:29 +00:00

(Migrated from gitlab.com)

I applied @timokoesters's suggestion

CobaltCause commented

2023-12-27 04:14:30 +00:00

(Migrated from gitlab.com)

I replaced this whole thing with slice patterns, which also avoids re-slicing the slice again (even though LLVM could most likely have eliminated those redundant bounds checks anyway).

CobaltCause commented

2023-12-27 04:17:01 +00:00

(Migrated from gitlab.com)

Note that this change affects the ordering in the resulting name so that last is actually the last instead of the first. Not sure if that's going to break anything, which is why I'm highlighting this particular change.

Note that this change affects the ordering in the resulting `name` so that `last` is actually the last instead of the first. Not sure if that's going to break anything, which is why I'm highlighting this particular change.

timokoesters commented

2024-01-17 10:56:29 +00:00

(Migrated from gitlab.com)

Hmm that deprecation warning is only there to warn clients. Servers still need to provide the info I think?

timokoesters commented

2024-01-17 11:11:01 +00:00

(Migrated from gitlab.com)

Okay I thought about it and agree, so we would replace them with structs containing the parameters when we get the chance, correct?

girlbossceo commented

2024-01-17 17:52:25 +00:00

(Migrated from gitlab.com)

https://spec.matrix.org/v1.9/client-server-api/#post_matrixclientv3login says home_server shouldn't be sent by servers and the client should extract the server name from the user_id themselves to figure out where they belong.

Deprecated. Clients should extract the server_name from user_id (by splitting at the first colon) if they require it. Note also that homeserver is not spelt this way.

Conduit simply shouldn't be sending it as it's redundant now and clients will extract the server from user_id themselves, so the warning is correct. I haven't had any authentication issues with FluffyChat, Element Web, or Element X with this.

https://spec.matrix.org/v1.9/client-server-api/#post_matrixclientv3login says `home_server` shouldn't be sent by servers and the client should extract the server name from the `user_id` themselves to figure out where they belong. > Deprecated. Clients should extract the server_name from user_id (by splitting at the first colon) if they require it. Note also that homeserver is not spelt this way. Conduit simply shouldn't be sending it as it's redundant now and clients will extract the server from `user_id` themselves, so the warning is correct. I haven't had any authentication issues with FluffyChat, Element Web, or Element X with this.

CobaltCause commented

2024-01-17 18:51:01 +00:00

(Migrated from gitlab.com)

Thanks for considering this! As for dealing with the lint when it triggers, it depends I think. I imagine sometimes a good argument could be made as to why a function needs a bunch of arguments and a struct would be pointless overhead, but in general, yeah, making a struct is a good way to avoid this.

CobaltCause commented

2024-01-18 20:15:37 +00:00

(Migrated from gitlab.com)

Actually that doesn't compile lol so I'm just converting it back to how it was before. Honestly not totally sure why I bothered to change this in the first place

CobaltCause commented

2024-01-18 20:29:12 +00:00

(Migrated from gitlab.com)

added 14 commits

426ec25c...24759951 - 2 commits from branch famedly:next
24759951...7074ecfa - 2 earlier commits
f91fa484 - fix `cargo check` lints
496f731e - fix `cargo clippy` lints
0500a0dc - fix `cargo doc` lints
7ecfd366 - add an engage file
fe02e589 - improve nix flake
a81f41da - remove commented out ci step
2287315b - remove dockerlint step because it does nothing
f87dcb48 - remove workflow rules
db8fe5ff - use nix and engage to manage ci
4544ceb9 - update flake.lock

Compare with previous version

added 14 commits <ul><li>426ec25c...24759951 - 2 commits from branch <code>famedly:next</code></li><li>24759951...7074ecfa - 2 earlier commits</li><li>f91fa484 - fix `cargo check` lints</li><li>496f731e - fix `cargo clippy` lints</li><li>0500a0dc - fix `cargo doc` lints</li><li>7ecfd366 - add an engage file</li><li>fe02e589 - improve nix flake</li><li>a81f41da - remove commented out ci step</li><li>2287315b - remove dockerlint step because it does nothing</li><li>f87dcb48 - remove workflow rules</li><li>db8fe5ff - use nix and engage to manage ci</li><li>4544ceb9 - update flake.lock</li></ul> [Compare with previous version](/famedly/conduit/-/merge_requests/564/diffs?diff_id=898833434&start_sha=426ec25cf22cea13b4d0474c0b1febcf8634a74f)

CobaltCause commented

2024-01-18 21:08:32 +00:00

(Migrated from gitlab.com)

added 1 commit

e64a885d - use nix-filter to filter sources

Compare with previous version

added 1 commit <ul><li>e64a885d - use nix-filter to filter sources</li></ul> [Compare with previous version](/famedly/conduit/-/merge_requests/564/diffs?diff_id=898864068&start_sha=4544ceb9d8f5d06c83089371a21a343c7a7769b6)

CobaltCause commented

2024-01-18 21:43:45 +00:00

(Migrated from gitlab.com)

changed title from fix {-lints-} to fix {+CI+}

changed title from **fix {-lints-}** to **fix {+CI+}**

CobaltCause commented

2024-01-18 21:50:45 +00:00

(Migrated from gitlab.com)

added 3 commits

208e9747 - use nix and engage to manage ci
be732699 - update flake.lock
c632c082 - use nix-filter to filter sources

Compare with previous version

added 3 commits <ul><li>208e9747 - use nix and engage to manage ci</li><li>be732699 - update flake.lock</li><li>c632c082 - use nix-filter to filter sources</li></ul> [Compare with previous version](/famedly/conduit/-/merge_requests/564/diffs?diff_id=898894961&start_sha=e64a885daaf9d5002456d17ece2213cccb817ee6)

CobaltCause commented

2024-01-18 22:15:56 +00:00

(Migrated from gitlab.com)

I figured I should just take this to the finish line and enforce that the checks I just fixed don't immediately get violated again. I left relatively detailed commit messages about why I did certain things, so make sure to look at those too when reviewing.

Some things that were broken before are now just gone; for example, there is currently no automated Docker image building. I would be willing to fix this in a future MR though (this one is pretty big as it is).

I figured I should just take this to the finish line and enforce that the checks I just fixed don't immediately get violated again. I left relatively detailed commit messages about why I did certain things, so make sure to look at those too when reviewing. Some things that were broken before are now just gone; for example, there is currently no automated Docker image building. I would be willing to fix this in a future MR though (this one is pretty big as it is).

CobaltCause commented

2024-01-19 05:57:03 +00:00

(Migrated from gitlab.com)

(Side note: holy crap I just realized that it was me who pointed out this redundancy (and the misspelling of "homeserver") in the API nearly 7 years ago in https://github.com/matrix-org/matrix-spec-proposals/pull/1097 (they opened this PR on my behalf but I brought it up in one of the Matrix rooms first))

(Side note: holy crap I just realized that it was me who pointed out this redundancy (and the misspelling of "homeserver") in the API nearly 7 years ago in <https://github.com/matrix-org/matrix-spec-proposals/pull/1097> (they opened this PR on my behalf but I brought it up in one of the Matrix rooms first))

CobaltCause commented

2024-01-19 05:59:54 +00:00

(Migrated from gitlab.com)

Following the trail from that PR, it looks like well_known is supposed to be the better-documented non-deprecated replacement for home_server: https://github.com/matrix-org/matrix-spec-proposals/pull/1730

Following the trail from that PR, it looks like `well_known` is supposed to be the better-documented non-deprecated replacement for `home_server`: https://github.com/matrix-org/matrix-spec-proposals/pull/1730

jfowl commented

2024-01-19 06:26:33 +00:00

(Migrated from gitlab.com)

requested review from @jfowl

jfowl commented

2024-01-19 07:02:14 +00:00

(Migrated from gitlab.com)

I can't really say anything about the Rust changes, but if we could somehow leave docker image building in (be it by using the existing CI step or nix-native), this looks good to me.

jfowl commented

2024-01-19 07:03:51 +00:00

(Migrated from gitlab.com)

Note that the current docker step is also responsible for outputting the normal binaries and the .deb files along the way

Note that the current docker step is also responsible for outputting the normal binaries and the `.deb` files along the way

meenzen commented

2024-01-19 10:26:39 +00:00

(Migrated from gitlab.com)

I love how this basically makes the ci setup portable, it looks very easy to migrate to GitHub Actions for example

CobaltCause commented

2024-01-19 16:00:27 +00:00

(Migrated from gitlab.com)

Yep! And you can use Engage locally to tighten your feedback loop. When combined with using Nix locally too, you can be sure you have the exact same environment and checks that CI does, so if it works on your machine, it will work in CI too.

CobaltCause commented

2024-01-19 16:14:39 +00:00

(Migrated from gitlab.com)

I was mainly trying to ship an MVP CI system that just does tests and code quality checks in this MR. The plan was to handle that stuff in a follow-up MR, since this one is already quite large and I didn't want to make it too unwieldy to review. I figured we wouldn't be missing much in the time between merging this one and then the follow-up since that stuff was broken before anyway, and I have code for building the Docker images already so hopefully it shouldn't take me too long to do the rest. I could just do it in this MR though if that's what you want.

I was mainly trying to ship an MVP CI system that just does tests and code quality checks in this MR. The plan was to handle that stuff in a follow-up MR, since this one is already quite large and I didn't want to make it too unwieldy to review. I figured we wouldn't be missing much in the time between merging this one and then the follow-up since that stuff was broken before anyway, and I have code for building the Docker images already so hopefully it shouldn't take me too long to do the rest. I *could* just do it in this MR though if that's what you want.

CobaltCause commented

2024-01-19 22:05:51 +00:00

(Migrated from gitlab.com)

added 3 commits

490a5dbb - use nix and engage to manage ci
494a3af8 - update flake.lock
ed1cc6c3 - use nix-filter to filter sources

Compare with previous version

added 3 commits <ul><li>490a5dbb - use nix and engage to manage ci</li><li>494a3af8 - update flake.lock</li><li>ed1cc6c3 - use nix-filter to filter sources</li></ul> [Compare with previous version](/famedly/conduit/-/merge_requests/564/diffs?diff_id=900058245&start_sha=c632c08291c4aff1cf7ef3c952b39d162e2b057b)

CobaltCause commented

2024-01-20 04:42:08 +00:00

(Migrated from gitlab.com)

added 9 commits

f6970225 - improve nix flake
17776bf7 - remove commented out ci step
36240a62 - remove dockerlint step because it does nothing
2fdb66d8 - remove workflow rules
26f52dd6 - use nix and engage to manage ci
493a4fcf - update flake.lock
9f303300 - use nix-filter to filter sources
ae2c936e - add docker image and build it in ci
b2dfe04d - add debian package building in ci

Compare with previous version

added 9 commits <ul><li>f6970225 - improve nix flake</li><li>17776bf7 - remove commented out ci step</li><li>36240a62 - remove dockerlint step because it does nothing</li><li>2fdb66d8 - remove workflow rules</li><li>26f52dd6 - use nix and engage to manage ci</li><li>493a4fcf - update flake.lock</li><li>9f303300 - use nix-filter to filter sources</li><li>ae2c936e - add docker image and build it in ci</li><li>b2dfe04d - add debian package building in ci</li></ul> [Compare with previous version](/famedly/conduit/-/merge_requests/564/diffs?diff_id=900159945&start_sha=ed1cc6c3f3af2ec945b345b25b16cdcbcc0891d9)

CobaltCause commented

2024-01-20 04:46:10 +00:00

(Migrated from gitlab.com)

Okay, I've done a little rebasing and added two commits. One for building a Docker image and exposing it as an artifact in CI, and another for doing the same for a Debian package. These are both for x86_64 only, I haven't attempted cross compilation.

CobaltCause commented

2024-01-20 04:48:03 +00:00

(Migrated from gitlab.com)

added 2 commits

a2e6b7f0 - add docker image and build it in ci
43dcad68 - add debian package building in ci

Compare with previous version

added 2 commits <ul><li>a2e6b7f0 - add docker image and build it in ci</li><li>43dcad68 - add debian package building in ci</li></ul> [Compare with previous version](/famedly/conduit/-/merge_requests/564/diffs?diff_id=900160713&start_sha=b2dfe04d0a0dac23a3b90c2379b88d9c93b3f98a)

CobaltCause commented

2024-01-20 04:51:14 +00:00

(Migrated from gitlab.com)

Just going to get ahead of comments asking why I didn't use GitLab CI features instead of wrapping a bunch of stuff in if command -v nix > /dev/null; then ...; fi like this.

It's because YAML anchors didn't work and extends doesn't merge values. I don't like it either but this is the best we can do without duplicating code.

Just going to get ahead of comments asking why I didn't use GitLab CI features instead of wrapping a bunch of stuff in `if command -v nix > /dev/null; then ...; fi` like this. It's because YAML anchors didn't work and `extends` doesn't merge values. I don't like it either but this is the best we can do without duplicating code.

CobaltCause commented

2024-01-20 04:52:42 +00:00

(Migrated from gitlab.com)

requested review from @jfowl

CobaltCause commented

2024-01-20 04:58:12 +00:00

(Migrated from gitlab.com)

added 2 commits

ac8e71f9 - redo docker image and build it in ci
9e18bfed - add debian package building in ci

Compare with previous version

added 2 commits <ul><li>ac8e71f9 - redo docker image and build it in ci</li><li>9e18bfed - add debian package building in ci</li></ul> [Compare with previous version](/famedly/conduit/-/merge_requests/564/diffs?diff_id=900162308&start_sha=43dcad682bc305d5b9a736782b00482a0d234c82)

CobaltCause commented

2024-01-20 05:01:33 +00:00

(Migrated from gitlab.com)

added 2 commits

756b180a - redo docker image and build it in ci
42e1c9cd - add debian package building in ci

Compare with previous version

added 2 commits <ul><li>756b180a - redo docker image and build it in ci</li><li>42e1c9cd - add debian package building in ci</li></ul> [Compare with previous version](/famedly/conduit/-/merge_requests/564/diffs?diff_id=900162649&start_sha=9e18bfed834c657e4341e6b480a3c78e318316a1)

CobaltCause commented

2024-01-20 05:11:58 +00:00

(Migrated from gitlab.com)

mentioned in merge request !567

CobaltCause commented

2024-01-20 06:29:48 +00:00

(Migrated from gitlab.com)

added 1 commit

5d817b5b - add debian package building in ci

Compare with previous version

added 1 commit <ul><li>5d817b5b - add debian package building in ci</li></ul> [Compare with previous version](/famedly/conduit/-/merge_requests/564/diffs?diff_id=900174690&start_sha=42e1c9cdd6f982342c6668872533d2f686678918)

CobaltCause commented

2024-01-20 07:15:11 +00:00

(Migrated from gitlab.com)

I definitely kept trying to improve stuff over the last few hours but I think it's finally in a decent spot. I'll stop touching it for now so it can be reviewed. (And maybe merged? 👉 👈)

I definitely kept trying to improve stuff over the last few hours but I think it's finally in a decent spot. I'll stop touching it for now so it can be reviewed. (And maybe merged? :point_right: :point_left:)

meenzen commented

2024-01-20 18:27:42 +00:00

(Migrated from gitlab.com)

The debian and docker builds should probably only run on protected branches, you can do this by adding the following to the jobs:

  only:
    - next
    - master
    - tags

The debian and docker builds should probably only run on protected branches, you can do this by adding the following to the jobs: ```yml only: - next - master - tags ```

meenzen commented

2024-01-20 18:27:42 +00:00

(Migrated from gitlab.com)

I'm new to nix so I'm not sure how one would go about implementing multi arch builds. Not having arm64 images is a problem and definetely needs to be added in the future.

meenzen commented

2024-01-20 18:27:43 +00:00

(Migrated from gitlab.com)

I can't comment on the rust changes but the CI part looks good.

Publishing of the built binaries and docker images to the GitLab Registry is still missing. I'd be happy to open a MR for that once this is merged.

I can't comment on the rust changes but the CI part looks good. Publishing of the built binaries and docker images to the GitLab Registry is still missing. I'd be happy to open a MR for that once this is merged.

CobaltCause commented

2024-01-20 18:49:59 +00:00

(Migrated from gitlab.com)

On the other hand, I think it would be really useful to, for example, produce builds for MRs so that other users (at least, other Docker/Debian users) can easily test the changes themselves before merge if they want.

CobaltCause commented

2024-01-20 18:56:46 +00:00

(Migrated from gitlab.com)

Theoretically, running nix build .#oci-image on aarch64 should produce an aarch64 image without having to make any changes. Cross compilation is probably possible, but also probably high effort. I'm willing to look into this, but I don't have any aarch64 hardware to test on. I guess I could just use QEMU though.

Theoretically, running `nix build .#oci-image` on aarch64 should produce an aarch64 image without having to make any changes. Cross compilation is probably *possible*, but also probably high effort. I'm willing to look into this, but I don't have any aarch64 hardware to test on. I guess I could just use QEMU though.

meenzen commented

2024-01-20 19:29:19 +00:00

(Migrated from gitlab.com)

GitLab does not provide any arm runners so someone would need to host one. Can a maintainer please check if there's already any self hosted runners registered in the project settings?

GitLab does not provide any arm runners so someone would need to host one. Can a maintainer please check if there's already any self hosted runners registered in the [project settings](https://gitlab.com/famedly/conduit/-/settings/ci_cd?expand_runners=true)?

meenzen commented

2024-01-20 19:40:04 +00:00

(Migrated from gitlab.com)

For reference, this is how we can create a multi-arch manifest:

docker manifest create \
registry.gitlab.com/famedly/conduit/matrix-conduit:next \
--amend registry.gitlab.com/famedly/conduit/matrix-conduit:next-amd64 \
--amend registry.gitlab.com/famedly/conduit/matrix-conduit:next-arm64v8

docker manifest push registry.gitlab.com/famedly/conduit/matrix-conduit:next

see: https://www.docker.com/blog/multi-arch-build-and-images-the-simple-way/

For reference, this is how we can create a multi-arch manifest: ```bash docker manifest create \ registry.gitlab.com/famedly/conduit/matrix-conduit:next \ --amend registry.gitlab.com/famedly/conduit/matrix-conduit:next-amd64 \ --amend registry.gitlab.com/famedly/conduit/matrix-conduit:next-arm64v8 docker manifest push registry.gitlab.com/famedly/conduit/matrix-conduit:next ``` see: https://www.docker.com/blog/multi-arch-build-and-images-the-simple-way/

CobaltCause commented

2024-01-20 21:19:21 +00:00

(Migrated from gitlab.com)

I'm saying aarch64 users can still build docker images on their own if they want right now. As for doing builds in CI, I think I'd rather get cross-compilation working.

jfowl commented

2024-01-21 21:15:50 +00:00

(Migrated from gitlab.com)

GitLab does not provide any arm runners so someone would need to host one. Can a maintainer please check if there's already any self hosted runners registered in the project settings?

Currently, there are only amd64 runners available, but it would be possible to get access to an arm64 one.

Indirectly, it is possible to SSH to a 4CPU-ARM64 server in the Oracle Cloud, which was used to build the docker image natively on there.

I would favor cross-compilation too though, as it appears to work (for dynamically linked builds) reliably via cargo-zigbuild.

> GitLab does not provide any arm runners so someone would need to host one. Can a maintainer please check if there's already any self hosted runners registered in the [project settings](https://gitlab.com/famedly/conduit/-/settings/ci_cd?expand_runners=true)? Currently, there are only amd64 runners available, but it would be possible to get access to an arm64 one. Indirectly, it is possible to SSH to a 4CPU-ARM64 server in the Oracle Cloud, which was used to build the docker image natively on there. I would favor cross-compilation too though, as it appears to work (for dynamically linked builds) reliably via cargo-zigbuild.

jfowl commented

2024-01-21 21:22:11 +00:00

(Migrated from gitlab.com)

I don't know NIX, so this is a bit hard for me to review, but it looks ok.

@eisfunke 👉 👈 ?

I'm fine with breaking docker image publishing for next for the moment, as long as we can get it working again for a real release.

Breaking artifact links would need an update to DEPLOY.md though.

Overall: Looks good

I don't know NIX, so this is a bit hard for me to review, but it looks ok. @eisfunke :point_right: :point_left: ? I'm fine with breaking docker image publishing for `next` for the moment, as long as we can get it working again for a real release. Breaking artifact links would need an update to `DEPLOY.md` though. Overall: Looks good

meenzen commented

2024-01-21 21:40:10 +00:00

(Migrated from gitlab.com)

There's no need to ssh into the server, you can just install GitLab runner on there and run the ci job natively.

https://docs.gitlab.com/runner/

There's no need to ssh into the server, you can just install GitLab runner on there and run the ci job natively. https://docs.gitlab.com/runner/

CobaltCause commented

2024-01-22 16:32:42 +00:00

(Migrated from gitlab.com)

Just an update about cross compilation, I got cross to x86_64-unknown-linux-musl fully working here last night. I haven't tried aarch64 yet, but I think getting static builds is the harder part anyway.

Just an update about cross compilation, I got cross to `x86_64-unknown-linux-musl` fully working [here](https://or.computer.surgery/charles/rust-rocksdb-musl-nix/-/tree/no-build-script?ref_type=heads) last night. I haven't tried aarch64 yet, but I think getting static builds is the harder part anyway.

CobaltCause commented

2024-01-22 16:48:32 +00:00

(Migrated from gitlab.com)

Okay so for aarch64-unknown-linux-musl, I made the obvious changes and immediately hit this rust issue but this nixpkgs PR tells me that switching to the lld linker specifically for this platform should fix it?

Okay so for `aarch64-unknown-linux-musl`, I made the obvious changes and immediately hit [this rust issue](https://github.com/rust-lang/rust/issues/89626) but [this nixpkgs PR](https://github.com/NixOS/nixpkgs/pull/278798) tells me that switching to the `lld` linker specifically for this platform should fix it?

CobaltCause commented

2024-01-23 05:07:05 +00:00

(Migrated from gitlab.com)

Update: I have cross to aarch64-unknown-linux-musl working! Code here. The caveat is that I disabled all of the rocksdb crate's features in order to do this, and Conduit uses a few of them, so I'll need to figure that out next.

Update: I have cross to `aarch64-unknown-linux-musl` working! [Code here](https://or.computer.surgery/charles/rust-rocksdb-musl-nix/-/tree/no-build-script?ref_type=heads). The caveat is that I disabled all of the `rocksdb` crate's features in order to do this, and Conduit uses a few of them, so I'll need to figure that out next.

CobaltCause commented

2024-01-23 05:37:50 +00:00

(Migrated from gitlab.com)

I tried armv7l-unknown-linux-musleabihf for fun and it actually failed to build RocksDB's C library, I think upstream doesn't support that target: https://github.com/facebook/rocksdb/issues/8609. I won't pursue this target any further for now.

I tried `armv7l-unknown-linux-musleabihf` for fun and it actually failed to build RocksDB's C library, I think upstream doesn't support that target: <https://github.com/facebook/rocksdb/issues/8609>. I won't pursue this target any further for now.

CobaltCause commented

2024-01-23 22:25:45 +00:00

(Migrated from gitlab.com)

The previous cross compilation stuff I was doing were just tests using buildRustPackage from nixpkgs and a dummy project that depended on rocksdb. After more fiddling, I have cross compilation of Conduit itself to x86_64-unknown-linux-musl fully working here, but I wasn't so lucky with aarch64-unknown-linux-musl, for which I've opened an issue on Crane since I suspect it's a bug there: https://github.com/ipetkov/crane/issues/497.

The previous cross compilation stuff I was doing were just tests using `buildRustPackage` from nixpkgs and a dummy project that depended on `rocksdb`. After more fiddling, I have cross compilation of Conduit itself to `x86_64-unknown-linux-musl` fully working [here](https://gitlab.com/CobaltCause/conduit/-/tree/cross?ref_type=heads), but I wasn't so lucky with `aarch64-unknown-linux-musl`, for which I've opened an issue on Crane since I suspect it's a bug there: <https://github.com/ipetkov/crane/issues/497>.

CobaltCause commented

2024-01-24 06:06:02 +00:00

(Migrated from gitlab.com)

Update: I have cross to x86_64-unknown-linux-musl and aarch64-unknown-linux-musl here!

The next problem is making doing these builds in CI efficient.

Update: I have cross to `x86_64-unknown-linux-musl` and `aarch64-unknown-linux-musl` [here](https://gitlab.com/CobaltCause/conduit/-/tree/cross)! The next problem is making doing these builds in CI efficient.

timokoesters commented

2024-01-24 11:47:16 +00:00

(Migrated from gitlab.com)

I agree that builds should only be made after the changes have been reviewed and merged.

timokoesters commented

2024-01-24 12:11:35 +00:00

(Migrated from gitlab.com)

I can approve the Rust part of this MR.

timokoesters commented

2024-01-24 12:11:40 +00:00

(Migrated from gitlab.com)

approved this merge request

timokoesters commented

2024-01-24 12:13:54 +00:00

(Migrated from gitlab.com)

Nico in #spec said the server must still send the field

CobaltCause commented

2024-01-24 14:58:52 +00:00

(Migrated from gitlab.com)

I'm curious why not?

Another point to consider here is that an MR might break cross compilation but if we aren't doing builds for MRs then we won't notice until it's too late.

I'm curious why not? Another point to consider here is that an MR might break cross compilation but if we aren't doing builds for MRs then we won't notice until it's too late.

CobaltCause commented

2024-01-24 15:23:04 +00:00

(Migrated from gitlab.com)

changed this line in version 13 of the diff

changed this line in [version 13 of the diff](/famedly/conduit/-/merge_requests/564/diffs?diff_id=903942606&start_sha=5d817b5b54cd74939695a26bdee197b785ac8a85#e244fe262da2c45f451ed9471527b37a4f0c6c6a_164_164)

CobaltCause commented

2024-01-24 15:23:07 +00:00

(Migrated from gitlab.com)

changed this line in version 13 of the diff

CobaltCause commented

2024-01-24 15:23:07 +00:00

(Migrated from gitlab.com)

added 13 commits

5d817b5b...0d17aeda - 3 earlier commits
e8ac881b - add an engage file
6f052fff - improve nix flake
25ceb5eb - remove commented out ci step
9d592d60 - remove dockerlint step because it does nothing
ffd03a25 - remove workflow rules
7e66d2e2 - use nix and engage to manage ci
f8bdfd82 - update flake.lock
02781e4f - use nix-filter to filter sources
4de54db3 - redo docker image and build it in ci
0b7ed5ad - add debian package building in ci

Compare with previous version

added 13 commits <ul><li>5d817b5b...0d17aeda - 3 earlier commits</li><li>e8ac881b - add an engage file</li><li>6f052fff - improve nix flake</li><li>25ceb5eb - remove commented out ci step</li><li>9d592d60 - remove dockerlint step because it does nothing</li><li>ffd03a25 - remove workflow rules</li><li>7e66d2e2 - use nix and engage to manage ci</li><li>f8bdfd82 - update flake.lock</li><li>02781e4f - use nix-filter to filter sources</li><li>4de54db3 - redo docker image and build it in ci</li><li>0b7ed5ad - add debian package building in ci</li></ul> [Compare with previous version](/famedly/conduit/-/merge_requests/564/diffs?diff_id=903942606&start_sha=5d817b5b54cd74939695a26bdee197b785ac8a85)

CobaltCause commented

2024-01-24 15:23:50 +00:00

(Migrated from gitlab.com)

Okay, I put this back to normal and added an #[allow(deprecated)] with a comment explaining why.

Okay, I put this back to normal and added an `#[allow(deprecated)]` with a comment explaining why.

CobaltCause commented

2024-01-24 15:29:31 +00:00

(Migrated from gitlab.com)

The plan is to add this stuff in a follow-up MR today.

timokoesters commented

2024-01-24 15:42:23 +00:00

(Migrated from gitlab.com)