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

Move examples to explicit gfx_support crate/module. Also move to virtual cargo workspace. #1417

Merged
merged 2 commits into from
Aug 17, 2017
Merged

Conversation

bjadamson
Copy link
Contributor

@bjadamson bjadamson commented Aug 5, 2017

I noticed this issue about gfx_app has pretty much stopped, when I was just looking, and realized the exact same thing had been confusing me trying to learn gfx. In the thread, it was brought up maybe it was a good idea to move gfx_app to it's own module, to help users with learning the API (whom are reading the example code).

Since progress has slowed down in the PR for cargo workspaces I thought I would submit this as some progress. I thought it was good timing since the last commit to the gfx repository just accepted a pull-request doing a similar rename, it seems smart to do another rename pull-request like this (since others maintaining branches will already need to rebase). I could be wrong about this, but the alternative would for me to sit back and wait until another point in the future to do this.

I think renaming gfx_app to gfx_support, and more importantly moving the code out of gfx's root (and under examples/) will help users understand what is "gfx library code" vs "gfx support library code". The distinction is subtle but important, I believe. It certainty wasn't clear to me until quite a while of digging by myself.

I have a few things I'd like to discuss moving forward, this pull request certainly isn't ready as is.

  1. I'd like to gather consensus that this is the right direction to move gfx. Are there any objections to the direction of this pull-request?
  2. Is gfx_project a good name for the "root project"?
  3. I'm not sure that duplicating the Cargo.toml file is the right way to express what I'm intending. Right now I'm duplicating the dependencies section in both Cargo.toml files. Apart from going through each dependency one-by-one, and seeing whether examples/ or gfx/ requires the dependency, is there a way to not duplicate the dependencies your aware of? Is duplicating them a problem?
  4. In the other thread glium was brought up as an example, in glium's example code, the helper code lives in the examples/support/ directory, causing all examples to have to add explicitly mod support it. I tried emulating the same setup, but I wasn't able to get it to compile. I would need to spend more time comparing with the glium project to emulate that setup.
  5. I need to try using the support code from another package, to make sure it's visible the way I think it should be. This is a TODO.

Anyways, I stole this list from the other thread. Here is what is working:

cargo build
cargo run --example (they all seem to work)

What is not working:

cargo test -- this isn't working for me on master/, not completely sure abouit this one.
cargo build --all (error: native frameworks are only available on macOS targets)
cargo test --all (error: Could not compile core-foundation-sys)

Pretty tired now when writing this up, so posting this looking to get some direction/guidance to move forward with this change.

edit:
Resolves:

@msiglreith
Copy link
Contributor

msiglreith commented Aug 5, 2017

Hi and thanks for that opening this PR!

I was just looking, and realized the exact same thing had been confusing me trying to learn gfx

I had a similar experience when trying to understand the repository structure. Therefore I'm also in favor of reorganizing the crates.

I could be wrong about this, but the alternative would for me to sit back and wait until another point in the future to do this.

Seems perfectly fine to do it now! Even though I don't have a strong opinion for gfx_app or gfx_support. If we move towards support, we could also think about abandoning the current gfx_app (edited) crate as the backend-specific setup code became quite small after the window changes.

Is gfx_project a good name for the "root project"?

What would be the root project? IMO we should move towards a workspace based structure and clearly separate the crates. e.g

- examples
- backends
--- gl
--- dx11
--- vulkan
- gfx (current core)
- gfx_app/gfx_support (or merge this gfx_example)
- gfx_render
- window
--- dxgi
--- metal
--- glutin

Opinions?

@torkleyy
Copy link
Contributor

torkleyy commented Aug 6, 2017

I like that directory structure. Examples could probably just be a crate with many binaries.

@bjadamson
Copy link
Contributor Author

bjadamson commented Aug 6, 2017

That structure makes sense to me, but I'm seeing some blockers in the other thread that seemed to have halted progress. Does it make sense to use cargo workspaces if they can't pass the criteria? (the things not working in #1250.

Does it matter if cargo build --all and cargo test --all won't work for now?

@torkleyy
Copy link
Contributor

torkleyy commented Aug 6, 2017

I fixed that issue already IIRC.

@bjadamson
Copy link
Contributor Author

bjadamson commented Aug 6, 2017

edit: nvm I see now.

@torkleyy
Copy link
Contributor

torkleyy commented Aug 6, 2017 via email

@bjadamson
Copy link
Contributor Author

bjadamson commented Aug 7, 2017

Ok, I moved the directory structure around to match what @msiglreith proposed above.

One thing I need to request help with, I'm not sure how to best remove duplication between the root level Cargo.toml and gfx_support/Cargo.toml files. Both crate's need access to the cfg features, so it seems I need to declare the gl/dx/vulkan/etc... dependencies in both Cargo.toml files.. Does anyone have any ideas to only have to declare the list of "features" and dependencies once?

Here's exactly what I mean by duplication:
https://github.com/bjadamson/gfx/blob/mod-support/Cargo.toml#L12-L84
https://github.com/bjadamson/gfx/blob/mod-support/gfx_support/Cargo.toml#L30-L101

You'll notice in the two links, a subset of dependencies and config flags are duplicated. The flags seem really bad for DRY practices, maybe the duplicated dependencies list is unavoidable though?

What would be the root project? IMO we should move towards a workspace based structure and clearly separate the crates. e.g

That makes sense, but I think moving to a workspace structure is another step after this (if anything to reduce the scope of the amount I'm trying to learn all at once). That is unless you believe a workspace structure would help me with my duplication problem above?

My issue with using workspaces I'm having trouble finding any real documentation on cargo workspaces. Specifically, I could not find a good example repository to emulate for gfx. Because of this, I wasn't able to work through the errors I was getting trying to switch to using cargo workspaces.

edit: build succeeded

@msiglreith
Copy link
Contributor

Regarding workspaces you could take a look here for example. (shameless self-promotion..)
The root Cargo.toml only defines the repository hierarchy.

Summoning @kvark for opinions on the proposed structure and the overall direction

@bjadamson
Copy link
Contributor Author

Thank you for the link! I'll look through your repo (very nice logo btw) and see what I can accomplish.

@bjadamson
Copy link
Contributor Author

bjadamson commented Aug 7, 2017

I'm unsure the best way to move forward now. My last commit fully switches the project to using a cargo workspace, based off your example repo. I've gotten to the point where cargo build --all fails, but I can run the examples (trying to build packages previously controlled by cfg section of top-level Cargo.toml file).

Since previously the crate inclusion was controlled by the top level Cargo.toml file, I'm assuming we should use the --exclude flag for cargo @torkleyy implemented. I'm unsure how to control which packages get built, I doubt the user experience we want is to manually type the --exclude paths ourselves. What did you have in mind here?

I'm assuming we want to be able to say, "cargo build --all" and "cargo test -all", and have the --exclude's figured out by the compiler. Is this assumption correct? I'm not even sure if this is possible, without some build-time magic (sorry, still new to rust).

edit: I just checked the other pull-request, a virtual workspace is not used. No ideas to steal from there.

@kvark
Copy link
Member

kvark commented Aug 8, 2017

@bjadamson

I'd like to gather consensus that this is the right direction to move gfx. Are there any objections to the direction of this pull-request?

No objections here.

In glium's example code, the helper code lives in the examples/support/ directory, causing all examples to have to add explicitly mod support it.

We need to have it as a crate, not just a module, so other clients are able to use it.

Is gfx_project a good name for the "root project"?
I'm not sure that duplicating the Cargo.toml file is the right way to express what I'm intending. Right now I'm duplicating the dependencies section in both Cargo.toml files. Apart from going through each dependency one-by-one, and seeing whether examples/ or gfx/ requires the dependency, is there a way to not duplicate the dependencies your aware of? Is duplicating them a problem?

I don't think we need the root project at all. Duplicating contents of Cargo.toml doesn't make sense. We already made steps to have separate examples in glfw/sdl windows. We just need to push it all the way down:

  • trianglell will be inside gfx_core/examples
  • triangle will be inside gfx_render/examples
  • glfw/sdl examples will be left untouched
  • all other examples will be in gfx_support/examples

This strictly corresponds to what level of abstraction an example is based on.
Doing this would mean the root Cargo.toml has nothing but the list of workspace members, similar to https://github.com/servo/webrender/blob/master/Cargo.toml

Running examples would be a little less convenient, but that's a minor thing to worry about. Building everything would be done with cargo test --all --exclude XXX, where XXX depends on the platform.

@msiglreith

we could also think about abandoning the current gfx_core crate

This crate is only going to be used directly by a handful of clients, no need to strive making the name shorter (would only cause confusion).

@msiglreith
Copy link
Contributor

@kvark Sorry, I meant gfx_app not gfx_core (edited it now)

@kvark
Copy link
Member

kvark commented Aug 8, 2017

Dropping gfx_app completely would be great 👍
If there is some glue code still to be shared between examples, I'd be fine with glium-style support module as originally mentioned. My objection to it was based on the assumption that gfx_app still has some logic in it.

@bjadamson
Copy link
Contributor Author

everything would be done with cargo test --all --exclude XXX, where XXX depends on the platform

Generating XXX is what I've been trying to figure out the past few days. Previously before switching to virtual workspace, gfx uses cfg flags in the root Cargo.toml file, now that's not possible. Do you have an idea on how that would work?

Running examples would be a little less convenient, but that's a minor thing to worry about. Building everything would be done with cargo test --all --exclude XXX, where XXX depends on the platform.

Maybe this could be solved with a CLI tool? One you can install with Cargo, like some other projects have.

@kvark
Copy link
Member

kvark commented Aug 8, 2017

@bjadamson I agree it's rather annoying to not being able to just do cargo test --all. On the other hand, there is way more users of gfx-rs (who don't need to care about this) than developers. So we could start by just documenting the XXX in the README and then see if we can do more with a cargo plugin.

@bjadamson
Copy link
Contributor Author

Ok, I have moved the examples as you stated you would have them. The build is going to fail, right now I think that all is left is modifying the appvoyer config file

I need to think a bit on what it should be changed too, since test --all won't work at all, but we still want platform testing. I think.

@bjadamson
Copy link
Contributor Author

bjadamson commented Aug 9, 2017

I got the build to succeed, using this new appvoyer configuration. Could someone take a look and suggest any improvements?

I tried to make sure we're executing the same tests as before. It's just now I'm using gfx_support as the "build target", as it seems to consume all the other crates. I'm open to ideas that are better than setting your current working directory to the gfx_support directory and executing "cargo build"

@bjadamson
Copy link
Contributor Author

For some reason, the latest appvoyer execution didn't pick up my latest changes to the appvoyer config file. It was still executing using cargo test --all, which I changed. I'm pushing a README.md change to try and hopefully get appvoyer to pick up the newest config.

@bjadamson
Copy link
Contributor Author

Oops, I'm going to move the examples/ to gfx_support/examples

@bjadamson
Copy link
Contributor Author

bjadamson commented Aug 9, 2017

@bjadamson
Copy link
Contributor Author

bjadamson commented Aug 9, 2017

Weird, the build failed on two of the hosts, for a CMake error. Passed on the other two hosts though, I wonder if it's a fluke. Going to push dummy commit to force rebuild.

edit: nvm I see what's wrong.

@bjadamson bjadamson changed the title Move examples to explicit gfx_support crate/module. Move examples to explicit gfx_support crate/module. Also move to cargo workspace. Aug 9, 2017
@bjadamson bjadamson changed the title Move examples to explicit gfx_support crate/module. Also move to cargo workspace. Move examples to explicit gfx_support crate/module. Also move to virtual cargo workspace. Aug 9, 2017
@bjadamson
Copy link
Contributor Author

bjadamson commented Aug 9, 2017

Ok, I believe everything to be correct. It seems a known issue is preventing two of the hosts from executing the tests correctly. Going to see if they work tomorrow.

In the meantime, this is ready for review. Unless I think of anything else, I don't have any more changes to make. I've squashed all the commits into a single commit.

@msiglreith
Copy link
Contributor

Nice progress so far 👍
The build scripts can be simplified to something like
cargo test --all --exclude gfx_window_sdl --exclude gfx_device_metal --exclude gfx_device_metalll --exclude gfx_window_metal (example for windows), which should cover everything imo.

The current issue I see with our structure is that it's hard to find the examples. When I see a new project on github I usually check for a examples folder or test folder and take a quick look at the README to find a way to get started. Our usual examples are inside gfx_support which is probably unintuitive if you are not familiar with the project even though I understand why they should be put there.

@bjadamson
Copy link
Contributor Author

I agree.

One solution, not saying it's good, is to create an example/ directory in the root and put a small README.md file that explains to the user where the examples are.

@kvark
Copy link
Member

kvark commented Aug 9, 2017 via email

@bjadamson
Copy link
Contributor Author

README.md file added. Working on trying to find a version of cargo test --all with --exclude's that works.

@bjadamson
Copy link
Contributor Author

--exclude only seems to work on nightly. Do we want to use --exclude if it only works on nightly?

@kvark
Copy link
Member

kvark commented Aug 9, 2017

cargo test --all --exclude gfx_window_sdl --exclude gfx_device_metal --exclude gfx_device_metalll --exclude gfx_window_metal

wow, if this is really that long, we should have a helper, e.g. a Makefile.

@bjadamson
Copy link
Contributor Author

Specifying paths to the binaries though, I don't think I'm used to programming with rust enough yet to understand exactly what is meant by that. Could you elaborate a little bit?

@msiglreith
Copy link
Contributor

@bjadamson Specifying the path as done here.

@bjadamson
Copy link
Contributor Author

@msiglreith Oh ok, thanks.

@kvark
Copy link
Member

kvark commented Aug 16, 2017

@bjadamson sorry for not giving you an open path for finishing this PR. I'd hope to see this merged as much as you do.
Problem is - we are in a situation where no good solution for the directory structure satisfies everyone, and gaining making any changes to this design involves shuffling lots of stuff around (e.g. moving examples, renaming support, etc)...

The proposal @msiglreith linked to is a compromise in itself, given that I wanted to only have LL examples in the /examples folder and everything else in the corresponding projects. Nobody is happy about this, so it's hard to universally accept :)

That structure seems reasonable to me. If there's consensus I can make one last switch to that.

It doesn't appear that we found a better one, so let's accept this as a consensus.

Specifying paths to the binaries though, I don't think I'm used to programming with rust enough yet to understand exactly what is meant by that. Could you elaborate a little bit?

I mean having the examples as [[bin]] entries as opposed to [[example]], given that we are going into /examples folder to launch them anyway, and bin is shorter. See http://doc.crates.io/manifest.html

@bjadamson
Copy link
Contributor Author

Ok, I'm going to do that work. If another consensus comes up in the interim I'll be on gitter if you'd be kind enough to stop me (;

@bjadamson
Copy link
Contributor Author

bjadamson commented Aug 16, 2017

Oh one thing. I have two triangle examples. One using render, one use support. Since it is requested that there is one Cargo.toml file with all the examples laid out, they each need a unique name.

How should I name them? I see two strategies, but there are more I'm sure.

triangle_render
triangle?

This would mean all the support examples keep their same name.

or
triangle
triangle_support

This would involve renaming all the support examples to have a "_support " suffix. This is of course to maintain consistency.

edit: 3rd option.

triangle_render
triangle_support
cube_support
blend_support
etc...

@bjadamson
Copy link
Contributor Author

bjadamson commented Aug 16, 2017

Ok, I sent the build off. Assuming I didn't make any mistakes (or missing any documentation that needs another last minute update to point to the right directory) then it's ready minus the issue I mentioned above.

Here's the link though so it's concrete in case my description was unclear.
https://github.com/bjadamson/gfx/blob/mod-support/examples/Cargo.toml#L160-L163
https://github.com/bjadamson/gfx/blob/mod-support/examples/Cargo.toml#L151-L153

Just need an arbitrary naming convention for the examples that is consistent. I don't think triangle_render is particularly good, I'm just unsure if renaming all of the examples to have a "_support" suffix is desired other.

edit: squashed back down to two commits. One for each issue being resolved.

Copy link
Contributor

@msiglreith msiglreith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick update!
A few points left imo. Also gamma example should be in examples/render.

## Structure and current versions
`gfx` consist of several crates. You can find all of them in this repository.

| Core functionality: | Graphic backends: | Window backends: |
| :---: | :---: | :---: |
| [![gfx on crates.io](http://img.shields.io/crates/v/gfx.svg?label=gfx)](http://crates.io/crates/gfx) | [![gfx_device_gl on crates.io](http://img.shields.io/crates/v/gfx_device_gl.svg?label=gfx_device_gl)](http://crates.io/crates/gfx_device_gl) | [![gfx_window_sdl on crates.io](http://img.shields.io/crates/v/gfx_window_sdl.svg?label=gfx_window_sdl)](http://crates.io/crates/gfx_window_sdl) |
| [![gfx_app on crates.io](http://img.shields.io/crates/v/gfx_app.svg?label=gfx_app)](http://crates.io/crates/gfx_app) | [![gfx_device_dx11 on crates.io](http://img.shields.io/crates/v/gfx_device_dx11.svg?label=gfx_device_dx11)](http://crates.io/crates/gfx_device_dx11) | [![gfx_window_dxgi on crates.io](http://img.shields.io/crates/v/gfx_window_dxgi.svg?label=gfx_window_dxgi)](http://crates.io/crates/gfx_window_dxgi) |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this row removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was indicated earlier in the discussion that it was desirable to kill of gfx_app all together.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but what about the other crates?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't realize multiple items on one line. Fixed!

where `<my_dir>` is a directory name of your choice. Once gfx is downloaded you can build any of the gfx examples.
The examples are split across three directories, each pertaining to the GFX library they are using.

1. examples/core contains low level examples.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be examples/corell as we currently don't have pure core examples

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@@ -0,0 +1,67 @@
[package]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file seems outdated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops! File is entirely removed now.

@@ -0,0 +1,22 @@
[package]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file seems outdated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops! File is entirely removed now.

@@ -1,4 +1,4 @@
#version 120
#version 110
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this changed intended? If so, please rename the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, not for this PR. Reverted.

@@ -1,4 +1,4 @@
#version 120
#version 110
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still not fixed

@@ -1,4 +1,4 @@
#version 120
#version 110
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still not fixed

info/contrib.md Outdated
* _src/render_ : The main gfx package
* _src/backend : The different backends GFX supports.
* _src/core : gfx_core, core structures and the interface that backends must provide
* _src/corell : WIP, will be absorbed into src/core and be.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'and be.'
Not a native speaker, but I assume there is missing something.

Copy link
Contributor Author

@bjadamson bjadamson Aug 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll just get rid of the "and be". I was going to indicate something along the lines that corell and core are going to be merged.. but it doesn't seem useful now.

I'll put something about how there's no resource overhead compared to render, unless you have a better idea.


# TODO: remove
[dev-dependencies]
gfx = { path = "../render", version = "0.16" }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can remove this again?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I removed it. I can't run the vulkan examples obviously, but the behavior seems correct.

[features]
default = ["gl"]
gl = ["gfx_device_gl", "gfx_window_glutin"]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other backend features should be added here also.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, fixed. Downsides of testing on one platform.

@bjadamson
Copy link
Contributor Author

Allright, changes requested pushed. I'm wondering if the dependency on dxgi in support/ is necessary, but I don't have a windows box for another week to test it on. It complains that dx11 and dx12 require dxgi, so I just left it like that. Thanks for such careful reviews @msiglreith, I think I'm becoming blind to the mistakes when reviewing because I've looked at it to many times.

info/contrib.md Outdated
* _src/backend_ : The backends implementations
* _src/render_ : The main gfx package
* _src/backend : The different backends GFX supports.
* _src/core : gfx_core, core structures and the interface that backends must provide
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to be consistent here by either naming the crates explicitly for all entries, or omitting them for all (simpler).
Also, trailing point is missing at the end.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed trailing point, removed reference to gfx_core. Did I understand correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assuming thumbs up means I did

@@ -0,0 +1,163 @@
[package]
name = "gfx-examples"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be gfx_examples for consistency with other names

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

[package]
name = "gfx-examples"
version = "0.7.0"
description = "GFX example application framework"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need this updated, it's not a framework

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed (unless you want this kept I can add it back)

@@ -0,0 +1,163 @@
[package]
name = "gfx-examples"
version = "0.7.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be 0.1.0
also, quite a few of the fields are not needed here since this project is never going to be published

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed and I removed all non-essential fields (keep name, version, workspace)

keywords = ["graphics", "gamedev"]
license = "Apache-2.0"
authors = ["The Gfx-rs Developers"]
documentation = "https://docs.rs/gfx_project"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

broken link, better remove this at all

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

image = "0.15"

gfx = { path = "../src/render", version = "0.16" }
gfx_core = { path = "../src/core", version = "0.7.1" }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we actually need all the dependencies here?
I'd assume that we only need corell, render, support, window_glutin, and a few things for trianglell

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was able to remove:
gfx_macros

I needed to keep all the rest to build the examples, since they all use one Cargo.toml file now. Unless I'm missing something, I don't see how we would remove any more dependencies listed in this file and still have the examples runnable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! We can always clean up more later on.

@@ -1,4 +1,4 @@
#version 120
#version 110
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still not fixed

@@ -1,4 +1,4 @@
#version 120
#version 110
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still not fixed

@bjadamson
Copy link
Contributor Author

All fixed. Sorry about the 110 version not being fixed, I don't know what I did to undo that fix before pushing last time. Maybe it was my rebase, I'll be more careful.

@bjadamson
Copy link
Contributor Author

bjadamson commented Aug 16, 2017

Ok, I believe it's all in order finally. Except for the last squash. GNU windows build is the only CI that doesn't pass. I have no idea why it's failing to link only on that platform. The windows MSVC builds pass.

Could someone take a look and see if they understand what the cause is?

edit: url

I could add an if check to see if it's compiling GNU in the appvoyer script, and try excluding the dx12ll backend package, but that feels like a hack.

@msiglreith
Copy link
Contributor

Excluding it for GNU would be fine as there seems to be no support for d3d12 yet

@bjadamson
Copy link
Contributor Author

Ok, GNU excluded. Because of that, I was able to put in "cargo test --all" into the appvoyer builds. So that should be cargo test --all for each platform.

Syntax is a bit wieldly though, yml is weird and I don't see an obvious way to reduce repetition and maintain if/else branching.
https://github.com/bjadamson/gfx/blob/mod-support/appveyor.yml#L18-L56

@bjadamson
Copy link
Contributor Author

Oh, also gained this line, I assume it's desirable to add. I'm not exactly sure how test --all and --features flags work. I'm not sure if the three lines testing dx11 dx12 and dx12ll are redundant with cargo test --all.
https://github.com/bjadamson/gfx/blob/mod-support/appveyor.yml#L56

Based on discussion in pull-request
#1417 re-arranges project structure.
Goals are to increase new developer discoverability and project
maintainence over time.

Examples are moved within associated crates, ensuring examples are
coupled with their abstraction level.
@bjadamson
Copy link
Contributor Author

bjadamson commented Aug 17, 2017

Looks like all the tests passed. The host running the "linux nightly" compiler seems to have died mid run, I'm pushing a noop update to force the CI to rerun.

Is there a better way to get the travis hosts to rerun when this happens?

Anyways, I think it's ready.

edit: tests passed!

Copy link
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I got a few more notes, but nothing worth holding this PR any longer
thanks for all the hard work and patience!

@@ -71,8 +71,8 @@ pub fn main() {
let views = swap_chain.create_color_views(&mut device);

let pso = device.create_pipeline_simple(
include_bytes!("shader/triangle_150.glslv"),
include_bytes!("shader/triangle_150.glslf"),
include_bytes!("shader/triangle_120.glslv"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doing that would make it fail on OSX, lt's leave GLSL 150 there

@@ -1,5 +0,0 @@
@echo off
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this file should probably be moved instead of removed

@@ -0,0 +1,111 @@
// Copyright 2017 The Gfx-rs Developers.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any idea why this file is marked as new as opposed to moved?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming because it didn't exist before hand. I rewrote the basic triangle example using the support module, so users could compare implementing a basic triangle with and without the support module.

gfx_device_gl = { path = "../backend/gl", version = "0.14", optional = true }
gfx_window_glutin = { path = "../window/glutin", version = "0.17", optional = true }

[dev-dependencies]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this whole section should no longer be needed

@kvark
Copy link
Member

kvark commented Aug 17, 2017

bors r+

bors bot added a commit that referenced this pull request Aug 17, 2017
1417: Move examples to explicit gfx_support crate/module. Also move to virtual cargo workspace. r=kvark

I noticed [this issue about gfx_app](#922) has pretty much stopped, when I was just looking, and realized the exact same thing had been confusing me trying to learn gfx. In the thread, it was brought up maybe it was a good idea to move gfx_app to it's own module, to help users with learning the API (whom are reading the example code).

Since progress has slowed down in [the PR for cargo workspaces](#1250) I thought I would submit this as *some* progress. I thought it was good timing since the last commit to the gfx repository just accepted a [pull-request doing a similar rename](d1e99ce), it seems smart to do another rename pull-request like this (since others maintaining branches will already need to rebase). I could be wrong about this, but the alternative would for me to sit back and wait until another point in the future to do this.

I think renaming gfx_app to gfx_support, and more importantly moving the code out of gfx's root (and under examples/) will help users understand what is "gfx library code" vs "gfx support library code". The distinction is subtle but important, I believe. It certainty wasn't clear to me until quite a while of digging by myself.

I have a few things I'd like to discuss moving forward, this pull request certainly isn't ready as is.
1. I'd like to gather consensus that this is the right direction to move gfx. Are there any objections to the direction of this pull-request?
2. Is gfx_project a good name for the "root project"?
3. I'm not sure that duplicating the Cargo.toml file is the right way to express what I'm intending. Right now I'm duplicating the dependencies section in both Cargo.toml files.  Apart from going through each dependency one-by-one, and seeing whether examples/ or gfx/ requires the dependency, is there a way to not duplicate the dependencies your aware of? Is duplicating them a problem?
4. In the other thread glium was brought up as an example, in glium's example code, the helper code lives in the [examples/support/](https://github.com/glium/glium/tree/master/examples/support) directory, causing all examples to have to add  [explicitly mod support it](https://github.com/glium/glium/blob/master/examples/deferred.rs#L10). I tried emulating the same setup, but I wasn't able to get it to compile. I would need to spend more time comparing with the glium project to emulate that setup.
5. I need to try using the support code from another package, to make sure it's visible the way I think it should be. This is a TODO.

Anyways, I stole this list from the other thread. Here is what is working:

cargo build
cargo run --example (they all seem to work)

What is not working:

cargo test -- this isn't working for me on master/, not completely sure abouit this one.
cargo build --all (error: native frameworks are only available on macOS targets)
cargo test --all (error: Could not compile `core-foundation-sys`)

Pretty tired now when writing this up, so posting this looking to get some direction/guidance to move forward with this change. 

edit:
Resolves:
* #1248
* #1424
@bors
Copy link
Contributor

bors bot commented Aug 17, 2017

@bors bors bot merged commit 08b2624 into gfx-rs:master Aug 17, 2017
@bjadamson
Copy link
Contributor Author

Holy crap it happened. I'll try and take a look at your mentions in a few days (packing up my pc tonight to move).

What timing, thanks again for all the encouragement :)

@bjadamson bjadamson deleted the mod-support branch August 17, 2017 14:35
bors bot added a commit that referenced this pull request Aug 24, 2017
1444: Fix shader versions to work on macOS r=kvark a=grtlr

The shader versions were changed in #1417.
bors bot added a commit that referenced this pull request Aug 24, 2017
1444: Fix shader versions to work on macOS r=kvark a=grtlr

The shader versions were changed in #1417.
@xtian xtian mentioned this pull request Aug 25, 2017
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.

4 participants