Skip to content

feat: implement gl parsing logic#23

Open
pollend wants to merge 15 commits intokrolli:masterfrom
pollend:feat/implement-gl-parsing-logic
Open

feat: implement gl parsing logic#23
pollend wants to merge 15 commits intokrolli:masterfrom
pollend:feat/implement-gl-parsing-logic

Conversation

@pollend
Copy link

@pollend pollend commented Feb 27, 2022

this is a bit of a change to also support parsing: https://github.com/KhronosGroup/OpenGL-Registry

I moved some things into a common utility to so I can share logic between vulkan and opengl xml. decided to replicate the types since the xml differs just enough where it won't quite align.

@pollend pollend marked this pull request as draft February 27, 2022 23:49
@krolli
Copy link
Owner

krolli commented Mar 3, 2022

Hmm looks like quite an undertaking. I haven't looked into changes deep enough yet (and it will probably take me a few days until I can). Do you think it will make more sense to maintain both parsers in the same repository, or fork and use VK parser as base for GL?

@pollend
Copy link
Author

pollend commented Mar 3, 2022

Hmm looks like quite an undertaking. I haven't looked into changes deep enough yet (and it will probably take me a few days until I can). Do you think it will make more sense to maintain both parsers in the same repository, or fork and use VK parser as base for GL?

I was debating about that. feature flags can just drop it from the compilation so it might as well not be there. probably easier to maintain then splitting it into a separate repository. what is your view on this. you can split it up into separate package in one repository but then you have to publish them separately. I haven't published any packages on cargo so I'm not sure how much work it would be.

@krolli
Copy link
Owner

krolli commented Mar 5, 2022

Neither compilation nor publishing worries me, to be honest. Compilation can, as you said, drop one or the other using feature flags. Publishing is straightforward and I don't do it often enough to be worried about it.
I am mostly curious about maintenance and possible divergence of the two source formats. I haven't looked it in-depth yet, so I don't know what is same and what is different between the two. Also, what is same right now might not necessarily remain the same in the future. I think it can all stay together for now and we'll see whether it complicates maintenance over time. Can always come back to this later.

@pollend
Copy link
Author

pollend commented Mar 5, 2022

Neither compilation nor publishing worries me, to be honest. Compilation can, as you said, drop one or the other using feature flags. Publishing is straightforward and I don't do it often enough to be worried about it. I am mostly curious about maintenance and possible divergence of the two source formats. I haven't looked it in-depth yet, so I don't know what is same and what is different between the two. Also, what is same right now might not necessarily remain the same in the future. I think it can all stay together for now and we'll see whether it complicates maintenance over time. Can always come back to this later.

that's why all the type information is duplicated the only thing that is shared in parsing utilities that your wrote. can easily live as its own package could be moved to another crate how would you share the utilities between the two though? if you have the utilities in another crate do you need to publish that to cargo? maybe it does make sense to split this into its own crate :?

@krolli
Copy link
Owner

krolli commented Mar 6, 2022

Yep, all crates would have to be published to cargo.

@pollend
Copy link
Author

pollend commented Mar 6, 2022

Yep, all crates would have to be published to cargo.

that sounds kind of annoying. umm yea, would need to just duplicate some of those common utilities. I guess that is not too bad really.

@krolli
Copy link
Owner

krolli commented Mar 7, 2022

I think it's better to leave it all in one crate for now.

@pollend
Copy link
Author

pollend commented Mar 12, 2022

this is what I've been doing in the meantime with these changes: https://github.com/pollend/glew-rs

@pollend pollend marked this pull request as ready for review March 12, 2022 17:04
@krolli
Copy link
Owner

krolli commented Apr 8, 2022

I finally got around to looking at the changes you've made (sorry it took so long). For the most part, it looks very much the way I expected it to, which is definitely a good thing. There are only two things that stand out to me:

  1. This one is minor, but please, put the ci crate back in and move tests and all test dependencies there. I want to keep the main crate as lean as possible and all of this test stuff physically separate. This is something I would like to see done before merging the PR.

  2. Renaming of the crate is definitely going to complicate things. There are also other outdated fields in Cargo.toml that will need to be updated. There aren't many users, but those that exist will have to go out of their way to find out that the crate name is different. We will need to decide what to do about this before we publish new version. Since this would be breaking change, I think best might be to just find the high profile ones and do the update for them.

@pollend
Copy link
Author

pollend commented Apr 8, 2022

I finally got around to looking at the changes you've made (sorry it took so long). For the most part, it looks very much the way I expected it to, which is definitely a good thing. There are only two things that stand out to me:

1. This one is minor, but please, put the `ci` crate back in and move tests and all test dependencies there. I want to keep the main crate as lean as possible and all of this test stuff physically separate. This is something I would like to see done before merging the PR.

2. Renaming of the crate is definitely going to complicate things. There are also other outdated fields in Cargo.toml that will need to be updated. There aren't many users, but those that exist will have to go out of their way to find out that the crate name is different. We will need to decide what to do about this before we publish new version. Since this would be breaking change, I think best might be to just find the high profile ones and do the update for them.

I'll try make those changes on the weekend. you can create a separate branch and publish that but keep the old package?

@krolli
Copy link
Owner

krolli commented Apr 8, 2022

I actually don't know what will happen when I try to publish update to a crate that includes change of name. Maybe it will magically do what I want it to ... but I have doubts.
I may try and mess around with it on the side (some other crate) to see what it does.

@pollend
Copy link
Author

pollend commented Apr 8, 2022

I actually don't know what will happen when I try to publish update to a crate that includes change of name. Maybe it will magically do what I want it to ... but I have doubts. I may try and mess around with it on the side (some other crate) to see what it does.

at worst you create a new repository. I'm assuming crates keys off the package name and its reserved when you use it.

@MarijnS95
Copy link
Contributor

at worst you create a new repository.

Why? A crate is published under the name that is specified in Cargo.toml. The "crate name" in the canonical sense.

@pollend pollend force-pushed the feat/implement-gl-parsing-logic branch from 9d6e63d to 8e41a74 Compare April 10, 2022 18:58
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.

3 participants