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

[WIP] Use cargo workspace. Move gfx_app in its own crate #1250

Closed
wants to merge 1 commit into from
Closed

[WIP] Use cargo workspace. Move gfx_app in its own crate #1250

wants to merge 1 commit into from

Conversation

ama0
Copy link
Contributor

@ama0 ama0 commented May 2, 2017

This is a first prototype on using cargo workspace for the whole project. (#1248)
gfx_app is no longer the 'main crate'. It is moved into src/gfx_app

I left the old dependencies in the main Cargo.toml to make it easier to compare and see what may be a cause of error.

What is working:

  • cargo build
  • cargo run --example [all examples work]
  • cargo test

What is not working:

@ama0 ama0 changed the title Use cargo workspace. Move gfx_app in its own crate WIP Use cargo workspace. Move gfx_app in its own crate May 2, 2017
@kvark kvark changed the title WIP Use cargo workspace. Move gfx_app in its own crate [WIP] Use cargo workspace. Move gfx_app in its own crate May 2, 2017
@homu
Copy link
Contributor

homu commented May 4, 2017

☔ The latest upstream changes (presumably #1251) made this pull request unmergeable. Please resolve the merge conflicts.

@torkleyy
Copy link
Contributor

@ama0 Since I presumably created lots of conflicts with my formatting PR, may I try to do this workspace-PR? Or would you prefer to do it yourself?

@ama0
Copy link
Contributor Author

ama0 commented May 11, 2017

@torkleyy Sure, go on 👍 Just remember that there's still this blocking issue

@torkleyy
Copy link
Contributor

@ama0 Maybe we could just do a -p on each while the issue remains unsolved?

@ama0
Copy link
Contributor Author

ama0 commented May 11, 2017

@torkleyy I don't know what -p means in this context :(

@torkleyy
Copy link
Contributor

@ama0 From cargo build --help:

    -p SPEC, --package SPEC ...  Package to build

@ama0
Copy link
Contributor Author

ama0 commented May 11, 2017

@torkleyy I'm not sure how this would solve the build --all issue. I would just update this PR to the newest dependency changes in gfx, gfx_app and all examples to keep the changes to master as small as possible and wait for a fix in cargo.

@torkleyy
Copy link
Contributor

Well, I'm just working on the cargo issue, but you have to be patient - not a cargo expert 😄

@torkleyy
Copy link
Contributor

My PR is in this week's TWIR. IIRC the next release should be in 2 weeks.

@ama0
Copy link
Contributor Author

ama0 commented Jun 6, 2017

@torkleyy Congratulations 👍

I've updated this PR so it stays closer to current master. Currently it looks like this:

  • main crate is gfx with its code in src/render/src/lib.rs (There's no Cargo.toml in src/render/.. anymore, so dependencies have to point to to the root crate
  • gfx_app is now in src/gfx_app

There are still some issues I'm not sure how to solve since I'm not that deep into gfx internals.

  • window/glfw/Cargo.toml and window/sdl/Cargo.toml have a dependency on the renderer. This leads to a cyclic dependency since both are also dependencies of gfx. I commented both entries to make it build.

@msiglreith
Copy link
Contributor

window/glfw/Cargo.toml and window/sdl/Cargo.toml have a dependency on the renderer. This leads to a cyclic dependency since both are also dependencies of gfx. I commented both entries to make it build.

They shouldn't depend on render, this should be fixed if possible.

@ama0
Copy link
Contributor Author

ama0 commented Jun 8, 2017

I removed the changes to .gitignore (see: #1248 (comment)) and squashed the little change in contrib.md into the main commit.

@homu
Copy link
Contributor

homu commented Jun 11, 2017

☔ The latest upstream changes (presumably 50f6715) made this pull request unmergeable. Please resolve the merge conflicts.

@torkleyy
Copy link
Contributor

Okay, the flag landed on stable.

@kvark
Copy link
Member

kvark commented Jul 21, 2017

@ama0 would you be able to rebase this?

@ama0
Copy link
Contributor Author

ama0 commented Jul 23, 2017

Thanks for the heads up. I'll try to look into it in a few days.

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
@torkleyy
Copy link
Contributor

Guess this can be closed now that another PR implemented it.

@bjadamson
Copy link
Contributor

bjadamson commented Aug 17, 2017

anyone in the future: #1417

@kvark
Copy link
Member

kvark commented Aug 17, 2017

Thanks everyone, this is done!

@kvark kvark closed this Aug 17, 2017
adamnemecek pushed a commit to adamnemecek/gfx that referenced this pull request Apr 1, 2021
1258: Lint all the things r=kvark a=kvark

**Connections**
Fixes gfx-rs#1250

**Description**
Refactors the code to satisfy the linter, configures it, and enables on CI.

**Testing**
`cargo clippy` saves the dau

Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
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.

6 participants