gnuradio / volk

The Vector Optimized Library of Kernels
http://libvolk.org
GNU Lesser General Public License v3.0
557 stars 202 forks source link

Changes that suggested on #770 #771

Closed Zaryob closed 3 weeks ago

Zaryob commented 2 months ago

Writing to the python helper script moved to python/volk_modtool/CMakeLists.txt

jdemel commented 2 months ago

I'm trying to understand, why you want to move this code to another file. I'm not against moving it. It just seems to be unnecessary and I want to make sure we're not breaking anything.

UPDATE: I'm looking at the issue #770 . I'm still wondering what the issue might be here? Is the CMake variable set incorrectly? That might imply that smth in our CMake logic is broken and needs to be fixed.

Zaryob commented 2 months ago

Mainly I make this:

As I see that when I try to build this structured project to out/build/x64-Debug directory (also tried on Linux) python script that generated via submodules/volk/cmake/Modules/VolkPython.cmake will expected to state in out/build/x64-Debug but it generated in out/build/x64-Debug/submodules/volk

It seems that CMAKE_BINARY_DIR will differ while compiling. And this change fixed this problem.

If any other suggestions we can try.

jdemel commented 2 months ago

CMAKE_CURRENT_BINARY_DIR would possibly be the preferred way to use. I'd suspect changing this variable in the original location would already fix your issue. That'd be great. That way we can keep the code in a more generic place that makes it easier to re-use. Also, thanks for your patience.

jdemel commented 2 months ago

About our CI. The "flyci" part needs to be removed and will be soon, but the DCO checker should be satisfied. If you click on it, it will give you tailored instructions on how to fix it.

Zaryob commented 2 months ago

It wouldn't worked to use CMAKE_CURRENT_BINARY_DIR

1> [CMake] Project Source Directory: C:/Users/Zaryob/source/repos/XXXXX/core/submodules/volk 1> [CMake] Build Directory: C:/Users/Zaryob/source/repos/XXXXX/out/build/x64-Release/core/submodules/volk 1> [CMake] Current Source Directory: C:/Users/Zaryob/source/repos/XXXXX/core/submodules/volk 1> [CMake] Current Binary Directory: C:/Users/Zaryob/source/repos/XXXXX/out/build/x64-Release/core/submodules/volk

Main problem is directories stated above gives same time with "file(WRITE ..." runs. But other functions on VolkPython.cmake run time. and their CMAKE_CURRENT_BUILD_DIR and CMAKE_BUILD_DIR prespective is totally different

jdemel commented 1 month ago

@Zaryob I'd like to merge your PR as is but I can't because of the DCO. We adopted the DCO where every commit is signed off to verify that contributors are not blocked by anything.

In general this is achieved with git commit -s. The DCO check comes with a tailored message on how to fix it:

To add your Signed-off-by line to every commit in this branch:

    Ensure you have a local copy of your branch by [checking out the pull request locally via command line](https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/checking-out-pull-requests-locally).
    In your local branch, run: git rebase HEAD~2 --signoff
    Force push your changes to overwrite the branch: git push --force-with-lease origin main

The last line assumes that origin is the name of your remote github repository.

jdemel commented 3 weeks ago

Thanks for the follow-up! Assuming all the tests pass, I'd like to merge your PR.