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

Build not dependent on code generation #84

Closed
Skepfyr opened this issue Aug 9, 2019 · 4 comments
Closed

Build not dependent on code generation #84

Skepfyr opened this issue Aug 9, 2019 · 4 comments

Comments

@Skepfyr
Copy link
Contributor

Skepfyr commented Aug 9, 2019

The way the code generation is currently set up means that it is possible to change the generation code, not build it and end up with the project in an inconsistent state. While this situation is unlikely, it would be really hard to spot (although it would be easy to fix). There are a few ways I can think to deal with this:

  1. Document it, simplest and easiest, this problem is unlikely to come up so adding it to the contribution guidelines and/or a PR template would probably be enough.
  2. Add a CI check to make sure that the source doesn't change, this is (probably) foolproof and doesn't require any restructuring although I have no idea how easy it would be to do.
  3. Restructure the code generation to generate it during the build process. This is the most complex option and could slow down downstream compilations but has the added benefit of getting rid of the weird triple build you have to do if you modify the autogen. It would require some care so that it builds correctly when used as a dependency but it is probably possible.

I was leaning towards (3) (mostly because I mostly motivated by wanting to stop the triple build) when I started to write this but it has some quite strong downsides that would need to be investigated first.

@kvark
Copy link
Member

kvark commented Aug 9, 2019

I like (2) - it's simple to put in: just check that git diff rspirv is empty after the code gen.

@antiagainst
Copy link
Collaborator

Agreed with @kvark. I think we can start with (2) first. Not opposing to (3) too. It's just right now the autogen stuff needs external SPIRV-Headers repo is used by both the spirv_headers create and rspirv crate. I'm not exactly sure how to structure it.

@Jasper-Bekkers
Copy link
Collaborator

Fwiw, as a user of this library I wouldn't be a huge fan of (3). This library already drags in a whole bunch of extra dependencies through num & num-derive that I'm not super fond of.

@kvark
Copy link
Member

kvark commented Aug 22, 2019

has the added benefit of getting rid of the weird triple build you have to do if you modify the autogen

Shouldn't it be double-build (instead of triple)?
Also, I think this is less of a problem now as #90 landed - autogen is fast enough that it's not a problem to run it twice. Only affects developers, which are... just 3.5 people at the moment :)

@Skepfyr Skepfyr closed this as completed Aug 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants