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

Improve pipeline #149

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Improve pipeline #149

wants to merge 4 commits into from

Conversation

9SMTM6
Copy link

@9SMTM6 9SMTM6 commented Jul 20, 2024

This PR does a few things. If one of these makes you uncomfortable, feel free to ask me to remove them.

  • It removes all traces of action-rs. Update CI deployment #139 only removed one of these uses.
    • its using dtolnay/rust-toolchain to pull the toolchain.
    • this action doesn't support minimal, but honestly I also dont see the purpose of this since the action caches the toolchain, so removing this option speeds up previous tasks that used minimal, since they use the same toolchain as the other tasks
  • it removes wasm32 specific tasks where possible, e.g. its using --all-targets for cargo check
    • actually reading cargo documentation, --all-targets doesn't do that. So I re-enabled it, and updated it for trunk, with some explainer comments.
  • it removes usage of cross where its not required, which should speed up the pipeline.
  • it re-adds support for amd64 macos
  • it uses Swatinem/rust-cache everywhere, not just to build pages. To do that properly in the build step, even with cross, it sets the key field
  • it replaces mentions of eframe_template, which break the pipeline on anyone using this template, with ${{ github.event.repository.name }}, which will work as long as the crate is renamed to its github repostory name.

@9SMTM6
Copy link
Author

9SMTM6 commented Jul 20, 2024

this properly fixes #134 .

@9SMTM6
Copy link
Author

9SMTM6 commented Jul 20, 2024

We could maybe also use the suggested actions-rust-lang/setup-rust-toolchain. Earlier experiments for this did not work for me, however this was maybe because it uses Swatinem/rust-cache under the hood, so with my fix it may work again.

That action is however, for better or worse, a lot more complicated. It not only adds the cache (with less conservative settings by default than it uses on its own) but also matchers for cargo build, cargo fmtandcargo clippy` failures. And it is young, we'll have to see if this sticks around in contrast to actions-rs. dtolnay meanwhile is arguably the MVP of the general rust community.

@9SMTM6
Copy link
Author

9SMTM6 commented Jul 20, 2024

@patrickelectric this PR contains all the things I mentioned.

.github/workflows/rust.yml Outdated Show resolved Hide resolved
@9SMTM6
Copy link
Author

9SMTM6 commented Jul 20, 2024

@patrickelectric the reason you removed amd64 macos support was because of build failures, correct? If it was runtime errors than this will not have fixed the issue.

@patrickelectric
Copy link
Contributor

@patrickelectric the reason you removed amd64 macos support was because of build failures, correct? If it was runtime errors than this will not have fixed the issue.

I don't recall at the moment, I only have an aarch64 mac. Maybe it was lost during my refactory.

BTW, did you figure out how to get the musl builds ?

@9SMTM6
Copy link
Author

9SMTM6 commented Jul 20, 2024

@patrickelectric I did not really figure out musl builds. I followed a bit in your footsteps and observed the amount of issues. I also found my own little problems. On my system I use mold linker by default, and cross seems to read my systemwide ~/.cargo/config.toml that sets this. Which also causes issues in cross. So I kind of gave up on it, admittedly without too much experimentation.

The other issue with musl is that its (supposedly) slower than glibc in many situations, so since my app might do performance intensive stuff I was just fine with removing musl.

Also, I am not entirely sure what -crt-static does. As others said, perhaps statically linking wayland isnt a particularly good idea for compatibility, and if -crt-static does this this could easily lead to the binary not working on other systems depending on their wayland version.

regarding MacOS, here is your commit if that wakes things: patrickelectric@202a29a

And I cannot test MacOS, since I don't have a Mac at all. But from the nature of changes I did bet that the reason it was failing was of a build nature.

If you only have an arm64 mac, then the only way you could have known that it fails at runtime would've been that someone else told you, so unless you remember it it was probably build (?).

@9SMTM6
Copy link
Author

9SMTM6 commented Jul 20, 2024

I could perhaps spin up a amd64 mac VM with quickget. I did that in the past, its doable, but takes time and storage from my laptop, so unless required I'd like to spare myself that and bet that this works, unless proven wrong by someone else.

@patrickelectric
Copy link
Contributor

patrickelectric commented Jul 20, 2024 via email

@patrickelectric
Copy link
Contributor

I could perhaps spin up a amd64 mac VM with quickget. I did that in the past, its doable, but takes time and storage from my laptop, so unless required I'd like to spare myself that and bet that this works, unless proven wrong by someone else.

If you have the binaries available in your github fork action I can test it.

@9SMTM6
Copy link
Author

9SMTM6 commented Jul 20, 2024

If you have the binaries available in your github fork action I can test it.

My fork from the template doesn't run actions, and I don't really feel like debugging that. I linked above to my actual egui app, which isn't that far along, but if it works on your PC it works. If it doesn't work it may be for all kinds of other reasons (e.g. using wgpu), then I will probably invest in debugging why github doesn't run actions on my actual fork.

Here is the link again: https://github.com/9SMTM6/mcmc-demo/releases/tag/main

@patrickelectric
Copy link
Contributor

If you have the binaries available in your github fork action I can test it.

My fork from the template doesn't run actions, and I don't really feel like debugging that. I linked above to my actual egui app, which isn't that far along, but if it works on your PC it works. If it doesn't work it may be for all kinds of other reasons (e.g. using wgpu), then I will probably invest in debugging why github doesn't run actions on my actual fork.

Here is the link again: 9SMTM6/mcmc-demo@main (release)

You need to enable it:
https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/enabling-features-for-your-repository/managing-github-actions-settings-for-a-repository

@9SMTM6
Copy link
Author

9SMTM6 commented Jul 20, 2024

Thats the first place I looked. It allows all actions etc from all sources, so that's not it.

Could please just try it with the executable I provided? Its the same foundations.

@9SMTM6
Copy link
Author

9SMTM6 commented Jul 20, 2024

Regarding the musl build. After I just rename my global cargo config (that IMO should not be used anyways) I can execute the build just fine: RUSTFLAGS="-Ctarget-feature=-crt-static" cross build --target x86_64-unknown-linux-musl --release.

But I can not test it. I now understand what this flag does. It removes the static linking of musl, which means that this dynamically linked musl can use dlopen.

Thing is, the reason I use cross, is because I have no dynamically linkable musl present. So this fails. If I try to execute the resulting binary I get:

> ./target/x86_64-unknown-linux-musl/release/mcmc_demo   
zsh: no such file or directory: ./target/x86_64-unknown-linux-musl/release/mcmc_demo

I do damply remember that this is an amazingly horrible sideeffect of dynamic linking failing. Another experiment:

ldd ./target/x86_64-unknown-linux-musl/release/mcmc_demo                                   
./target/x86_64-unknown-linux-musl/release/mcmc_demo: error while loading shared libraries: /usr/lib/libc.so: invalid ELF header

so I'm pretty sure that it tries to load musl libc, but it gets glibc, and then realizes that it doesnt like glibc.

And I cant simply try this in a docker container, since the point of all this is to get a GUI with wayland in the end, and at least I don't know and don't feel like figuring out how to pass that through. Only idea I MIGHT be tempted to try is distrobox.

@9SMTM6
Copy link
Author

9SMTM6 commented Jul 20, 2024

The binary we get out in the end would not be an universal linux binary anyways (which it previously was). Because of this precise issue of dynamically linked musl libc. So it would be yet another target. Since we currently already target the architectures amd64, arm64 and armv7 on Linux, this would multiply this with 2 (glibc, musl), leading to 6 different linux targets.

I'm not sure if @emilk really wants to maintain that.

@9SMTM6
Copy link
Author

9SMTM6 commented Jul 29, 2024

@patrickelectric here is a link to the action with download, if you were concerned about it coming from a release.

Regarding musl, did you get my concerns? I'll try to explain it, note that my understanding of this is also limited, so I may be mistaken.

The reason the binary was build in musl, is because musl can be statically linked. With musl libc statically linked that binary can be used both in glibc based linux (most of them) and musl based linux (Alpine, a few others, Android also uses musl but theres probably other stuff missing).

But it appears that Wayland support, at least how its currently implemented in winit, requires the use of dlopen, and musl doesn't seem to support dlopen if statically linked.

If we pass -crt-static (note the -) then apparently the musl toolchain does switch to dynamically linking musl, which is by default linked statically for the musl toolchain. I did not find any definite documentation on that, but considering the context that is the logical conclusion. As such it will support dlopen, and will work in Wayland (probably).

However in that case, because we have no statically linked libc, the binary will look for a libc on the system, and because it was built with dynamic musl, it will expect a musl libc. However it gets what glibc provides. Which is apparently a GNU ld script when I look it up right now, not directly a .so file.

Its amazing how horrible the errors can be if you dynamically link against stuff thats not there. I had difficulties finding this properly described, but here is at least a reddit post.

Anyways. Because of this, if we want to support both glibc as well as musl based systems with wayland, we would either need to build for both separately, or figure out how to get around the dlopen issue, which seems way beyond the scope of this.

I'd say that in light of this, in light of most distros currently switching to Wayland by default and xWayland probably (?) causing issues with stuff like scaling, and the market share of alpine based distributions for Desktop PC usage being very small, I'd drop support for musl. Even if we think we added support for it, we would need to test it, preferably with some frequency.

@9SMTM6
Copy link
Author

9SMTM6 commented Jul 29, 2024

@emilk sorry for this amount of spam in here. I should've moved the whole wayland and musl discussion elsewhere.

I'm not sure why there was no reaction, I hope that you've either just been busy or primarily waiting for a confirmation of Mac compatibility from @patrickelectric.

At least parts of this have to be addressed at some point. If you look at the summary of your last action and scroll down, you will see a bunch of warnings about usage of deprecated calls caused by action-rs tools.

Doing this will also speed things up if caching is accepted. If the cache is hit, my pipeline runs in typically <2min, most builds run in ~30s. There seems to be some issue with particularly Windows and sometimes MacOS marking libraries as Fresh and sortof rebuilding them, but it still runs much faster and is better for the environment due to less work.

There is also still a considerable market share of amd64 macs, so supporting them is IMO not insignificant.

@patrickelectric
Copy link
Contributor

Hi @9SMTM6, sorry I was in a working trip. Will check the PR and artifacts when possible.

@9SMTM6
Copy link
Author

9SMTM6 commented Sep 2, 2024

@patrickelectric have you gotten around to testing that stuff yet? IDK how long github keeps these artifacts. Since then the pipeline of my project changed further away from the PR here, in ways that probably don't make sense to adopt.

In case it gets deleted I uploaded the artifact here.

@armandas
Copy link

Oh man, I spent a few hours today updating the actions for #169 😅
Hopefully when this is merged, I can make my PR smaller.

@9SMTM6
Copy link
Author

9SMTM6 commented Dec 1, 2024

@armandas I was kindof waiting for confirmation by @patrickelectric of this running on Macos. However a few days ago I got myself an Apple Silicon Mac Mini, and after bypassing Apples completely unacceptable 'security measures' (rather actually measures to control app distribution on macos like they do on ios), I was able to run the application on that apple silicon mac. Both the native arm64 version, as well as amd64 (with the compatibility layer installed).

@emilk is this acceptable? Since the point in time I made this PR, my project pipeline has diverged further from this pipeline, though I don't think most of these changes make sense to adopt here. But before the merge I might want to have another look over this, to see if there are some changes that would make sense. However, I have yet to receive any sign of life form you, so I don't know if this PR is something you want at all, so before I do that I'd like some confirmation that that work wont be for naught.

EDIT: BTW, the bypass is documented here https://www.idownloadblog.com/2024/08/07/apple-macos-sequoia-gatekeeper-change-install-unsigned-apps-mac/

OreQr added a commit to OreQr/video-editor that referenced this pull request Jan 5, 2025
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.

3 participants