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

remove nc_complex submodule, add source files directly #1337

Merged
merged 7 commits into from
Jun 18, 2024

Conversation

jswhit
Copy link
Collaborator

@jswhit jswhit commented Jun 14, 2024

the following files from nc_complex are added

external/nc_complex/src/nc_complex.c
external/nc_complex/include/nc_complex/nc_complex.h
external/nc_complex/include/generated_fallbacks/nc_complex_version.h

see issues #1329, #1331 and #1332

@aaraney
Copy link

aaraney commented Jun 15, 2024

Linking relevant conversation in #1332 here. #1332 (comment)

I really don't think it's a good idea to vendor in this way without some sort of upstream synchronization and checked in vendored version provenance information.

@jswhit
Copy link
Collaborator Author

jswhit commented Jun 16, 2024

I've included a README with provenance info (the source files come from https://github.com/PlasmaFAIR/nc-complex/releases/tag/v0.2.0). It only a single source file with header. Don't really see how this is any different from a submodule that points to a specific hash that needs to be updated to stay in sync with changes upstream.

@ZedThree
Copy link
Contributor

@jswhit Apologies, I've been on holiday the last couple of weeks so I missed all the fun with the submodules!

If vendoring nc-complex simplifies things, then it's probably the most sensible way forward, it just complicates updating it slightly -- although I'm not expecting a great deal of future updates.

I would say though, that it would be best to include a generated nc_complex_version.h with the exact commit hash used. You've noted the version in the README, but including the hash would be even better.

One option for the future would be to move the build system to scikit-build and make sure the submodules are initialised. Probably a github action could be used to uploaded a full tarball on releases too.

@aaraney
Copy link

aaraney commented Jun 17, 2024

@ZedThree, yeah you could take inspiration from pytorch.

@ZedThree
Copy link
Contributor

@aaraney Thanks, I know a couple of other projects that could use that too!

@jswhit
Copy link
Collaborator Author

jswhit commented Jun 17, 2024

@jswhit Apologies, I've been on holiday the last couple of weeks so I missed all the fun with the submodules!

If vendoring nc-complex simplifies things, then it's probably the most sensible way forward, it just complicates updating it slightly -- although I'm not expecting a great deal of future updates.

I would say though, that it would be best to include a generated nc_complex_version.h with the exact commit hash used. You've noted the version in the README, but including the hash would be even better.

One option for the future would be to move the build system to scikit-build and make sure the submodules are initialised. Probably a github action could be used to uploaded a full tarball on releases too.

@ZedThree how is nc_complex_version.h generated? I used the one from v0.2.0 tag, but it says version 0.1.0 in it.

@jswhit
Copy link
Collaborator Author

jswhit commented Jun 18, 2024

@jswhit Apologies, I've been on holiday the last couple of weeks so I missed all the fun with the submodules!
If vendoring nc-complex simplifies things, then it's probably the most sensible way forward, it just complicates updating it slightly -- although I'm not expecting a great deal of future updates.
I would say though, that it would be best to include a generated nc_complex_version.h with the exact commit hash used. You've noted the version in the README, but including the hash would be even better.
One option for the future would be to move the build system to scikit-build and make sure the submodules are initialised. Probably a github action could be used to uploaded a full tarball on releases too.

@ZedThree how is nc_complex_version.h generated? I used the one from v0.2.0 tag, but it says version 0.1.0 in it.

NVM I figured it out - it gets created by the cmake build.

@jswhit jswhit merged commit 4866651 into master Jun 18, 2024
37 checks passed
@jswhit jswhit deleted the include_nc-complex branch June 18, 2024 00:06
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