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

Re-enabling multiarch support #456

Merged
merged 22 commits into from Sep 26, 2022
Merged

Re-enabling multiarch support #456

merged 22 commits into from Sep 26, 2022

Conversation

martadinata666
Copy link
Contributor

@martadinata666 martadinata666 commented Jun 26, 2022

  1. Reenable multiarch build
  2. As we know github actions pretty limited, either killed OOM or 6hrs timeout

In this PR we do old way, build everything separately, then gather and push manifest. Some issue is the currecnt Dockerfile can't be used for this, as we will build js out side docker, and only build dim in container. I thought create Dockerfile.ci for this use case. Technically still like master Dockerfile but with UI build disabled.

Build proof :
https://github.com/martadinata666/dim/actions/runs/2565143979

Thoughts?

@vgarleanu
Copy link
Member

vgarleanu commented Jun 26, 2022

Couldn't we use matrix to do multi arch builds?

Also we don't push to dockerhub anymore, only ghcr.

Also I don't fully understand why building the UI is disabled. I'm not a fan of server binaries that don't embed the UI, which might confuse downstream users when they run dim.

@vgarleanu
Copy link
Member

Looking at the pipeline that you linked, the build times for different architectures is way too excessive. Why is this? I see that qemu is used in the workflow, although I'm not sure why. Are we emulating a different arch here?

Couldn't we make the multi arch build a lot simpler? Rust has very good support for cross compilation. We could just use something like cross.

@martadinata666
Copy link
Contributor Author

martadinata666 commented Jun 26, 2022

Im not really sure about the dynamic linking libraries, Will it work? Like build on libva amd64 and cross arm64 with libva arm64? Afaik it only work if it fully static like musl target.

@vgarleanu
Copy link
Member

As far as I know libva is either not supported on non-x86 hardware or there aren't any devices that support vaapi on non-x86 hardware, as such we should disable vaapi for other architectures.

That said I think we should still target musl for non-x86 architectures. What I had in mind is that we could use cross to compile the binaries for all other architectures targeting musl, then simply copy those binaries into the docker image.

I imagine most of the time in the arm job is spent compiling dim while also emulating arm on x86. This can be avoided as described above.

Since dim pretty much statically links everything, I doubt we will experience any hiccups here.

@martadinata666
Copy link
Contributor Author

martadinata666 commented Jun 27, 2022

So i played a bit, looking how cross played out in the end stuck on openssl sys issue:

There is downgrade method likely working?

workflow run:

and strategy on build image docker, cant really pass the matrix with special character, or maybe im missing something.
Another issue is if the matrix work, how it differentiate the tags, im not really sure.

  - name: Build and push based on matrix
        uses: docker/build-push-action@v2
        with:
          context: .
          push: ${{ github.event_name != 'pull_request' }} #push except PR.
          platforms: ${{ matrix.platforms }}
          tags: ${{ steps.metaid.outputs.tags }}
          labels: ${{ steps.metaid.outputs.labels }}

Update : matrix result oom killed, using matrix as it count as one job, i assume matrix still using single runner.

Im gonna revert to last working with nodejs build separately.

@martadinata666 martadinata666 marked this pull request as draft June 27, 2022 05:04
@vgarleanu
Copy link
Member

vgarleanu commented Jun 27, 2022

Openssl with musl will be tricky. I wonder which dependency is using it and if we can replace it with rustls.

p.s. you can amend your previous commit and force push instead of creating a lot of small commits for small fixes. Something like git commit --amend && git push -f should work well.

@martadinata666
Copy link
Contributor Author

martadinata666 commented Jun 27, 2022

openssl-sys break on gnu/musl target. such tricky issue. Im testing this and that 🤣 i hope some magic happens.

update:
https://github.com/martadinata666/dim/actions/runs/2569571677

docker build via artifact, maybe need to create release or so, but it your preferences.
https://github.com/martadinata666/dim/runs/7076707658

@vgarleanu
Copy link
Member

vgarleanu commented Jun 27, 2022

Looks pretty good so far. The pipeline run you posted seems to build binaries but not images. Although this should be easy to fix.

I looked at the aarch64 binaries and it seems like it still loads libc.so. Are we not using musl here?

p.s. I've got a raspi laying around, Ill try to test the binaries on it.

@vgarleanu
Copy link
Member

Also I'm not a fan of how the docker-ci workflow is a separate workflow. Could we combine it with the rust-target-multi workflow?

@vgarleanu
Copy link
Member

FYI I managed to get rid of openssl-sys on one of my branches. Turns out reqwest was using openssl instead of rustls by default.

@martadinata666
Copy link
Contributor Author

Yes, we can combine docker build with rust-target, currenctly just separated for faster testing.
The problem with alpine again about the SSL, im not really sure how to handle it, i think just change the target will do. 🤷🏼
Well we can just try it, to see where it fail.

Ref:
https://eighty-twenty.org/2019/10/15/cross-compiling-rust 🤞🏼

@vgarleanu
Copy link
Member

Just tried to compile targetting musl and it fails to link because of anitomy-sys using some glibc functions that musl doesnt implement (I think). Looks like we have to stay with glibc until we can fix anitomy

@vgarleanu
Copy link
Member

vgarleanu commented Jun 27, 2022

@martadinata666 We just need to replace

reqwest = { version = "0.11.0", features = ["json", "default-tls"], default-features = false }
with

reqwest = { version = "0.11.0", features = ["json", "rustls-tls"], default-features = false }

Doing this should get rid of any dependency on openssl.

@martadinata666
Copy link
Contributor Author

martadinata666 commented Jun 27, 2022

@martadinata666 We just need to replace

reqwest = { version = "0.11.0", features = ["json", "default-tls"], default-features = false }

with

reqwest = { version = "0.11.0", features = ["json", "rustls-tls"], default-features = false }

Doing this should get rid of any dependency on openssl.

Not really sure about this, do we need it? Is this for supporting ssl protocol out of the box without reverse proxy?
Will the rustls-tls do the same? We already can build with openssl, so i think it ok for now.

Otherhand, i agree just stay at glibc, this alpine musl, we could but via qemu, that defeating the purpose fast binary build. I will revert dim binary build and combine with docker build tomorrow. 👍🏼

@vgarleanu
Copy link
Member

vgarleanu commented Jun 27, 2022

I think we should still depend on rustls as I trust rust code more than C++ ;p. This will just affect the reqwest dependency which we use to fetch assets and make API calls. The api server already uses rustls (and supports tls without a reverse proxy).

@martadinata666
Copy link
Contributor Author

I see, moving to rusttls pretty much not an issue then. 👍🏼

Last build success: ✅
https://github.com/martadinata666/dim/actions/runs/2573606377

@martadinata666 martadinata666 marked this pull request as ready for review June 28, 2022 04:05
Dedy Martadinata Supriyadi and others added 2 commits June 28, 2022 12:49
@vgarleanu
Copy link
Member

This looks good. Will give this another quick review Monday and then I'm happy to merge!

@vgarleanu
Copy link
Member

Looks like building an aarch64 image is failing. The other CI jobs (docs / rust build) can be ignored

@martadinata666
Copy link
Contributor Author

Let me sync up with new commits and see the result, https://github.com/martadinata666/dim/actions/runs/3123000524

@martadinata666
Copy link
Contributor Author

Is it any specific reason that the tool chain locked in?

https://raw.githubusercontent.com/Dusk-Labs/dim/master/rust-toolchain

@vgarleanu
Copy link
Member

Yes, the toolchain is currently pinned because rustc releases newer than that version (both stable and nightly) fail to compile dim in release mode due to an ICE caused by some sort of MIR bug.

@vgarleanu
Copy link
Member

FYI if you rebase onto master, DATABASE_URL doesnt need to be set anymore, it will be automatically set by our build-scripts.

@martadinata666
Copy link
Contributor Author

Rebase to master again, and last build success, https://github.com/martadinata666/dim/actions/runs/3124534900

@vgarleanu vgarleanu merged commit bd87547 into Dusk-Labs:master Sep 26, 2022
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.

None yet

2 participants