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

Oni2 not usable when built from source. #2816

Closed
benwainwright opened this issue Dec 10, 2020 · 8 comments · Fixed by #2822
Closed

Oni2 not usable when built from source. #2816

benwainwright opened this issue Dec 10, 2020 · 8 comments · Fixed by #2822
Labels
A-packaging Area: Packaging per-platform bug Something isn't working

Comments

@benwainwright
Copy link

benwainwright commented Dec 10, 2020

Hey there!

So I wanted to have a go at contributing, except there is a bit of a snag...

You may remember that I opened #1185 a while back, and the issue was resolved. Well, it seems to be back, but oddly enough only when I build from source.

I've downloaded the latest nightly dmg from the website and that works fine. I've also confirmed it appears to be the same issue by unplugging my external monitor and trying the build from source version, and sure enough, when I do that, it works fine.

🤦

@bryphe
Copy link
Member

bryphe commented Dec 11, 2020

Hey @benwainwright ! Awesome, can always use extra help 😄

Well, it seems to be back, but oddly enough only when I build from source.

Interesting - part of the fix for this was to add a plist entry for the NSSupportsAutomaticGraphicsSwitching property (#2278)

For development builds, we're not creating a plist at all (we're only generating one on release here:

const plistContents = {
) - so it wouldn't have that setting. I think we may need an Info.plist in development builds too

@benwainwright
Copy link
Author

I think we may need an Info.plist in development builds too

Ok, well maybe that can be my first contribution. I'm a web/react developer by day and completely new to the reason/ocaml/dune/esy domain, so I'll be slow moving. But I've done some digging and read some docs and some of your code and I think I understand the following:

  • When you run esy build the output is the raw binaries without any packaging
  • Esy run simply runs the binary (from inside the esy sandbox) itself
  • esy "@release" create essentially packages together the actual MacOS app bundle

So if I understand this correctly, the only way to include an Info.plist in development builds would be to create an app bundle for development builds. This would either be done by adding an execution of another node script similar to release.js onto the end of the build invocation here:

"build": "refmterr dune build -p libvim,textmate,treesitter,Oni2 -j4",

or a separate script that people need to execute themselves, similar to the create script in release.json.

@bryphe
Copy link
Member

bryphe commented Dec 12, 2020

I'm a web/react developer by day and completely new to the reason/ocaml/dune/esy domain, so I'll be slow moving. But

Most of the contributors have a web background - that's a perfect skillset. Revery / Reason is like React taken to the extreme 😎 But the build / tool chain is quite different for sure!

So if I understand this correctly, the only way to include an Info.plist in development builds would be to create an app bundle for development builds

Actually, @zbaylin discovered we could just put an Info.plist alongside the executable to have some properties picked up (as part of exploring notifications on OSX): revery-ui/revery#672 Could potentially easier than having to add an app bundle.

I'll put together a prototype branch for that - we can give it a shot and see if that addresses the issue

@benwainwright
Copy link
Author

Great thanks!

@bryphe
Copy link
Member

bryphe commented Dec 12, 2020

Giving it a shot here: #2822

If you'd be up for trying it out, that would be a huge help:

  • git fetch origin
  • git checkout experiment/build/2816/development-plist

...we'll see if it fixes the issue 🤞

@bryphe
Copy link
Member

bryphe commented Dec 12, 2020

I think it's good to have it in either case - it means the development builds are closer to what we ship, anyway

@benwainwright
Copy link
Author

@bryphe Yup, that did the trick!

@bryphe
Copy link
Member

bryphe commented Dec 12, 2020

Awesome! Thanks @benwainwright

@bryphe bryphe added A-packaging Area: Packaging per-platform bug Something isn't working labels Dec 12, 2020
bryphe added a commit that referenced this issue Dec 12, 2020
Fix #2816 - Use a development `Info.plist` that specifies the `NSSupportsAutomaticGraphicsSwitching` setting.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-packaging Area: Packaging per-platform bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants