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

Update some info in the readme #62

Closed
wants to merge 2 commits into from
Closed

Conversation

bkirwi
Copy link
Collaborator

@bkirwi bkirwi commented Mar 28, 2021

As discussed in Dischord!

I dropped the entire musl section for now - we can always bring it back if we find instructions that work, but for now having it in seems confusing.

Also added a couple minor tweaks, like pointing to the wiki for the toolchain while that's in limbo.

@fenollp
Copy link
Collaborator

fenollp commented Mar 29, 2021

I think it can be useful to keep the section on cross just replacing mentions of musl with gnueabihf. This works in the CI today and does not necessitates the oecore toolchain nor special linker flags outside lto=true.

Not sure what's wrong with musl though!

What's the discussion you're mentioning? On which Discord?

@bkirwi
Copy link
Collaborator Author

bkirwi commented Mar 29, 2021

Apologies, here's the thread: https://discord.com/channels/385916768696139794/386181213699702786/824072591881797662

I'd be happy to have cross-based instructions, but I never quite got the whole thing to work, so I don't feel super qualified to write them! In the meantime it seemed useful to remove some content that was tripping people up.

My impression is that statically linking musl interacts badly with the framebuffer shim, which relies on being able to intercept libc calls that don't happen when you're not dynamically linking libc. @LinusCDE also mentioned some recent issue with evdev, though I'm not familiar.

@fenollp
Copy link
Collaborator

fenollp commented Mar 29, 2021

Alright I fixed building with musl (works for rM device v1) in #64 and hinted a bit more on cross usage.
Thanks for pointing out the problem I think this is now superseeded.

Regarding musl / the rm2fb shim / LD_PRELOAD / the rM2, it may work (quoting the musl FAQ) but I have not tested this (yet anyway):

The environment variables LD_PRELOAD and LD_LIBRARY_PATH are also supported for one-off cases.

@LinusCDE
Copy link
Collaborator

Regarding musl / the rm2fb shim / LD_PRELOAD / the rM2, it may work (quoting the musl FAQ) but I have not tested this (yet anyway):

Cool. Just tinkered around a bit and made a draft that can explicitly check for and call the for LD_PRELOAD intended functions of rm2fb.

LinusCDE@421babe

This would remove the requirement of setting LD_PRELOAD altogether. The draft is very basic though. It's very inefficient (always looking up the ioctl symbol again) and doing a lot of dirty debug printing I used for testing. I also didn't bother using rm2fb's close method (which could be problematic for some applications as rm2fb only ever expects one fd for /dev/fb0). But this may serve as a bases for explicitly using the lib. Feel free to do whatever you want with this poc. I personally don't plan on continuing on it in the foreseeable future and don't care what happens with that at all.

Here is a compiled version of the demo example. It'll work as long as the lib is present at /opt/lib/librm2fb_client.so.1. No LD_PRELOAD needed.

demo.zip

I also built a custom version of the rm2fb package that has lots of print debugging (source).

(cc @raisjn)

@LinusCDE
Copy link
Collaborator

Just gave @fenollp's musl build a try. The shim doesn't work on it indeed since no interpreter (usually some linux-ld library) is declared at all and this preventing any ld based interception. The above poc could for example only be enabled on musl builds to make them still work with the shim.

@fenollp
Copy link
Collaborator

fenollp commented Mar 29, 2021

Feel free to do whatever you want with this poc. I personally don't plan on continuing on it in the foreseeable future and don't care what happens with that at all.

Hey thanks for this. Is everything alright with you? your language seems kind of harsh.

This is a pretty sic PoC if you ask me :) I'll definitely continue your work.
Thoughts: rm2fb's close can maybe be called from libc::atexit and surely the two ioctl_func symbols can be resolved during lazy init or cached in some way.

@LinusCDE
Copy link
Collaborator

LinusCDE commented Mar 29, 2021

Hey thanks for this. Is everything alright with you? your language seems kind of harsh.

Sorry for that. Had a long day and wanted to make it clear that there is no issue in coping/using it at all. I'm sorry if that came forth as harsh. I didn't proof-read that message and it seems harsh indeed.

Thoughts: rm2fb's close can maybe be called from libc::atexit and surely the two ioctl_func symbols can be resolved during lazy init or cached in some way.

I gave lazily getting the ioctl_func a shot and ran into some weird issue that this specific static ref wasn't declared.

For the close I was more concerned that rust will issue a normal close once Framebuffer gets dropped. If a library would for some reason want to reopen it afterwards, rm2fb wouldn't have expected the fd to be closed. Implementing Drop for the framebuffer and doing a self.device.into_raw_fd() should probably prevent the file from getting closed.

A quick test also turned out that this won't work easily for a musl build. My guess is that libloader uses ld itself. Should we be able to add libloaders dependencies statically I'm not sure how this would be for loading rm2fb. It would probably require to at least load libc again anyway. Maybe creating a rm2fb rust crate with the option to use either a dynamically linked library (rust *-sys crate) or vendoring it would be useful for musl. I thinking of how openssl is done. You can specify the "vendored" feature to get it compiled into the rust binary.

As of now the poc can only serve to remove the need of using LD_PRELOAD. Not sure how useful that is.

EDIT: Giving the mentioned musl faq a look. It seems that we may just need to tweak something to get LD_PRELOAD to work again.

@bkirwi
Copy link
Collaborator Author

bkirwi commented Mar 30, 2021

That is definitely a very cool direction... seems like it would make adopting the library and cross-building for RM1 & RM2 much simpler. (From my perspective the main appeal of the musl instructions was the use of cross / not needing the "official" toolchain which is increasingly difficult to access. So I'd personally be excited by this approach even if it didn't improve the musl issue.)

Do folks think this PR is worth landing in the meantime? Or should I close and open a new issue for the LD_PRELOAD-less approach?

@LinusCDE
Copy link
Collaborator

Do folks think this PR is worth landing in the meantime? Or should I close and open a new issue for the LD_PRELOAD-less approach?

Yeah. I think that the LD_PRELOAD-less approach should be a different PR. For now having musl support is already a good thing. Even if this makes the framebuffer for the rM 2 useless in the musl-build. Maybe a mention regarding this problem could be added to the readme as well.

@fenollp
Copy link
Collaborator

fenollp commented Mar 30, 2021

From my perspective the main appeal of the musl instructions was the use of cross / not needing the "official" toolchain which is increasingly difficult to access

This is possible today (no musl involved). Works for rM1 & rM2:

cross build --example demo --release --target=armv7-unknown-linux-gnueabihf

This only requires cargo install cross (and Docker).


Maybe a mention regarding this problem could be added to the readme as well.

Mentioned in my PR. Thanks

@bkirwi
Copy link
Collaborator Author

bkirwi commented Apr 1, 2021

This is possible today (no musl involved)

Yep, figured this out eventually... but it wasn't clear when I was first learning this stuff a couple months back!

Even if this makes the framebuffer for the rM 2 useless in the musl-build. Maybe a mention regarding this problem could be added to the readme as well.

I restored the section on cross, but changed it to mention the glibc version first; it still mentions musl, but also that it's not working correctly on rm2 at the moment.

Maintainers should be allowed to edit this branch, so if there are tweaks to the wording you prefer feel free to make them directly!

@bkirwi
Copy link
Collaborator Author

bkirwi commented Apr 1, 2021

Ah, just saw the latest commits in #64, which covers this and more... will close this one down. Thanks for the discussion!

@bkirwi
Copy link
Collaborator Author

bkirwi commented Apr 1, 2021

Opened #65 as a tracking issue for the LD_PRELOAD-less approach!

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

3 participants