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

Simplify target dependencies #987

Merged
merged 1 commit into from
Jun 3, 2016
Merged

Simplify target dependencies #987

merged 1 commit into from
Jun 3, 2016

Conversation

eugene2k
Copy link
Contributor

@eugene2k eugene2k commented Jun 3, 2016

fixes #982

P.S. thx, pravic

@eugene2k
Copy link
Contributor Author

eugene2k commented Jun 3, 2016

On a similar note... what do you guys think of feature-gating sdl and glfw?

@kvark
Copy link
Member

kvark commented Jun 3, 2016

@eugene2k looks great, thanks!
Feature gating SDL and GLFW is the way to go. Would you want to implement it within this PR?

@eugene2k
Copy link
Contributor Author

eugene2k commented Jun 3, 2016

Might not need to feature-gate it at all. You said before that the reason why sdl was added as a dependency to gfx_app was so that everything could be tested with a single cargo test. That doesn't work anyway (see #961). So perhaps we should just remove sdl and glfw from gfx_app altogether.

@kvark
Copy link
Member

kvark commented Jun 3, 2016

The fact that this doesn't work is a bug (or missing feature) in cargo, not by design, IIRC. But I can't really see a use case where the app launcher would choose sdl or glfw over glutin, so your point still stands. I suppose we can remove sdl and glfw from gfx_app dependencies then, as you suggest.

@eugene2k
Copy link
Contributor Author

eugene2k commented Jun 3, 2016

There's a complication with that too. If the dependency is optional or not there, cargo can't find its manifest. So if you want to test say gfx_window_sdl you can't just run cargo test -p gfx_window_sdl in gfx root and have it tested. Instead you have to specify the manifest of the package (or cd to its directory) and then cargo will build not only the package itself, but also all of its dependencies even if you had already built them before, which will slow down travis/appveyor.

So, I guess you should probably merge the PR as it is...

@kvark
Copy link
Member

kvark commented Jun 3, 2016

Yeah, I've been worrying about this too. Let's leave this for now.
@homu r+

@homu
Copy link
Contributor

homu commented Jun 3, 2016

📌 Commit 5dfe598 has been approved by kvark

@homu homu merged commit 5dfe598 into gfx-rs:master Jun 3, 2016
@homu
Copy link
Contributor

homu commented Jun 3, 2016

⚡ Test exempted - status

homu added a commit that referenced this pull request Jun 3, 2016
Simplify target dependencies

fixes #982

P.S. thx, pravic
@eugene2k eugene2k deleted the 982 branch June 5, 2016 11:06
adamnemecek pushed a commit to adamnemecek/gfx that referenced this pull request Apr 1, 2021
987: Align stencil reference flags between pipeline creation and setting r=cwfitzgerald a=kvark

Sibling of gfx-rs#986 for master

Co-authored-by: Dzmitry Malyshau <dmalyshau@mozilla.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.

Simplify target dependencies
3 participants