search

NVIDIA / go-nvml

Go Bindings for the NVIDIA Management Library (NVML)
Apache License 2.0
311 stars 64 forks source link

Development workflow in `./gen/nvml` #97

Closed msanft closed 7 months ago

msanft commented 10 months ago

Hello there,

The README and the rules in the Makefile suggest the development of new bindings (when updating the header file) in the gen directory. On the other hand, the Go bindings are only created in the pkg directory, causing the gen directory, that hosts most of the wrapper code, to miss all of the definitions, mainly because nvml.go (i.e. the generation result) is not present there. Is there something I miss with the workflow, or would it just make a lot more sense to either have all of the definitions in gen and move everything excluding the gen-only files (i.e. nvml.yml) over on a release?

For now, I updated the bindings and the wrapper code in pkg only, which works. But if this should be upstreamed at some point, the whole repository and workflow should be kept in a good state probably.

Cheers, Moritz.

elezar commented 10 months ago

We have been discussing handling this better internally, but have not had the time to make the changes yet. Note that all go files in gen should be copied to pkg and as such changes made there should be reflected once make bindings has been run.

In the past there have been some issues with files in pkg being in the gitignore list since we don't want users to inadvertently add these.

In general, the suggestion is to:

  1. Make changes in gen
  2. Run make bindings to ensure that pkg is updated.

There may be a bug in the workflow, so if you're experiencing unexpected behaviour, please let us know.

msanft commented 10 months ago

Hey @elezar, thanks for the fast reply!

Overall I understand why you want to have the in-development gen directory and the redistributable pkg folder. The issue I'm facing with this approach is that nvml.go is not present in the gen directory, what causes the Go language server (and builds in gen) to miss a lot of definitions. I think you should be able to reproduce this by performing a fresh clone and trying to build or use an editor with a Go language server in gen.

I think this could be fixed by also generating bindings in the gen directory, so that Go can find all definitions when writing code in gen. These can also be cleaned up after a build if you don't want these to clutter gen. I don't think this is an optimal approach, but I think it would ease development a little.

elezar commented 10 months ago

The solution that I was considering was to move the development to pkg explicitly, and have the gen folder only contain the inputs for generated code.

I have created https://github.com/NVIDIA/go-nvml/pull/98 with a proposal. It's still a draft and I have not yet had a chance to update the readme or test the standard development flow, but feedback would be appreciated.

msanft commented 10 months ago

Hey @elezar, this would be the correct solution imo, but also the one involving more changes. (as you said, makefile and readme need to be updated here)

Please let me know if I can help with #98, and thanks a ton for your efforts!

elezar commented 10 months ago

Please let me know if I can help with #98, and thanks a ton for your efforts!

If you want to take over #98 (it's probably simplest to create your own PR) and make the additional required, I'd be happy to review them and get them in.