-
Notifications
You must be signed in to change notification settings - Fork 565
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
How to build instructions added #90
Conversation
Looks like something with nightly rust causing problems on CI |
CONTRIBUTING.md
Outdated
``` | ||
cargo build --target=x86_64-pc-windows-msvc | ||
``` | ||
Only windows and macos platforms are supported at the moment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
readme.md starting with it's windows-only. Is that outdated or to point that macos support is weaker?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We currently support macOS and windows approximately equally.
CONTRIBUTING.md
Outdated
@@ -3,6 +3,14 @@ | |||
We'd love to accept your patches and contributions to this project. There are | |||
just a few small guidelines you need to follow. | |||
|
|||
## How to build | |||
Fulfill [gtk-rs dependencies](http://gtk-rs.org/docs/requirements.html) for cairo crate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
readme.md states it's just cargo run, while without this dependency I'm getting compilation errors.
Remove the statement from readme.md?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mention of cargo run
in the readme is more referring to what is involved in getting a client application to run.
If you can't build on windows without specifying the target, could you open an issue? I'm not sure if that's a known thing or not.
In any case, I'm a bit confused: windows should be using d2d, so you shouldn't need the cairo dependency I don't think?
And related, the instructions added here are, at the very least, windows specific, so if we want to add this info it would be worth having separate ### windows
and ### macos
sections I think!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm using GNU/Linux (Ubuntu 18.4 dependencies, Mint 19) and cannot compile because my platform have no implementation. And for me the only way to find it out was to read the code and see, that it is not a dependency problem, there is no implementation at all and I have to specify supported platform explicitly.
Cairo currently used for mac builds. So if you will build binaries for mac on your windows machine, you'll need it, am I wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes okay that makes sense. :)
I think the solution to this problem is to clearly state in the readme what platforms are supported. We could also just deny compilation based on the current platform, e.g. something like,
#[cfg(not(any(target_os = "macos", target_os = "windows")))]
compile_error!("druid does not current support your platform.");
which would at least give a clear and immediate diagnostic? I'm not sure this is necessary though, it feels a bit cute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this small check make sense
In general, supporting GNU/Linux is something I'm planning to work on, but I would like to play around with it before declaring to maintain a platform. I'm new in Rust yet.
Regarding this docs, I will note that gtk-rs needed only for MacOs targeted builds, links at the bottom and general readme update regarding two platforms, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR updated
954c2b4
to
058061a
Compare
All this said, @Dmitry-Borodin if you're interested in working on linux support for druid we would be very happy to help, linux support is important but there just isn't anyone right now who is willing to own that platform and we don't want to have a half-supported platform blocking other work. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I think it makes sense to make these changes but I have some nitty suggestions. Thanks!
README.md
Outdated
@@ -5,7 +5,7 @@ | |||
Druid is a new Rust-native UI toolkit, still in early stages. Its main | |||
goal is performance, also aiming for small binary size and compile time, | |||
fast startup, and very easy build configuration (just `cargo run`). It |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would replace this sentence with, 'It current supports Windows and macOS, with GNU/Linux support planned.'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
CONTRIBUTING.md
Outdated
@@ -3,6 +3,13 @@ | |||
We'd love to accept your patches and contributions to this project. There are | |||
just a few small guidelines you need to follow. | |||
|
|||
## How to build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say,
Building
Currently, druid only builds on Windows and macOS.
macOS
On macOS, druid requires cairo; see [gtk-rs dependencies] for installation instructions.
You may also need to set your PKG_CONFIG_PATH
; assuming you have installed cairo
through homebrew, you can build with,
$> PKG_CONFIG_PATH="/usr/local/opt/libffi/lib/pkgconfig" cargo build
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little confused about the build instructions. My understanding is that we're asking people to install cairo on the mac build (ie brew install cairo
), not all of gtk. Just cairo is a considerably lighter weight dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we are mixing target platform and building platform.
Brew is only available in MacOS. But any platfrom, trying to build macos targeted binaries has to have hits dependencies.
So we cannot just ask here to use homebrew.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you talking about cross-compiling? I don't think we're supporting that, I can think of a number of ways in which that would be tricky, more so going forward (we may well want to depend on xcode-specific tools to, for example, pack resources into an app bundle).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, at least right now I can build for both platforms. But let's not support it officially until tested properly. I will update PR soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Readme updated
README.md
Outdated
@@ -5,7 +5,7 @@ | |||
Druid is a new Rust-native UI toolkit, still in early stages. Its main | |||
goal is performance, also aiming for small binary size and compile time, | |||
fast startup, and very easy build configuration (just `cargo run`). It | |||
is currently Windows-only, but with hope for porting. | |||
is currently Windows and MacOS only, porting to GNU/Linux in plans. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is currently spelled "macOS".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed everywhere
CONTRIBUTING.md
Outdated
@@ -3,6 +3,13 @@ | |||
We'd love to accept your patches and contributions to this project. There are | |||
just a few small guidelines you need to follow. | |||
|
|||
## How to build | |||
> Only windows and macos platforms are supported at the moment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Windows and macOS". Let's make sure the capitalization is consistent, there are a couple other instances.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay one last nit then I'm happy to merge this. 😎
original typo fixed Co-Authored-By: Colin Rofls <colin@cmyr.net>
Okay I'm happy to merge this shortly, although linux support may be landing sooner than expected! Anyway, thanks for the PR. :) |
No description provided.