emsec / hal

HAL – The Hardware Analyzer
MIT License
619 stars 74 forks source link

Cannot build from 4.1.0 source tarball #519

Closed xiota closed 1 year ago

xiota commented 1 year ago

Describe the bug Trying to build the 4.1.0 source tarball fails with "not a git repository" error.

To Reproduce

When running cmake, genversion.py fails upon calling git describe.

Configuration completes successfully when HAL_VERSION_MAJOR=4, HAL_VERSION_MINOR=1, and HAL_VERSION_PATCH=0 are passed to cmake. But build ultimately fails with:

[ 61%] Building CXX object app/CMakeFiles/hal.dir/main.cpp.o
In file included from /build/emsec-hal/src/hal-4.1.0/app/main.cpp:17:
/build/emsec-hal/src/hal-4.1.0/build/hal_version.h:47:27: error: expected primary-expression before ‘;’ token
   47 |         const int tweak = ;
      |                           ^
/build/emsec-hal/src/hal-4.1.0/build/hal_version.h:50:40: error: expected primary-expression before ‘;’ token
   50 |         const int additional_commits = ;
      |                                        ^
/build/emsec-hal/src/hal-4.1.0/build/hal_version.h:56:31: error: expected primary-expression before ‘;’ token
   56 |         const bool is_dirty = ;
      |                               ^
/build/emsec-hal/src/hal-4.1.0/build/hal_version.h:59:32: error: expected primary-expression before ‘;’ token
   59 |         const bool is_broken = ;
      |                                ^
make[2]: *** [app/CMakeFiles/hal.dir/build.make:76: app/CMakeFiles/hal.dir/main.cpp.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:1046: app/CMakeFiles/hal.dir/all] Error 2
make: *** [Makefile:136: all] Error 2

Expected behavior cmake should succeed without attempting to call git describe or having to set special version variables.

make should complete successfully.

Additional context Build guide should be updated with instructions to build from the source tarball.

390 describes additional variables to pass to cmake that may allow build to continue, but not going to test them because rebuilding takes too long. Also, the build scripts should not require building from a git repository.

joern274 commented 1 year ago

@xiota , you did a great job by testing the build from tarball and pointing out why exactly it is broken. There should be at least a fallback available if HAL version cannot be determined by git and we will fix it ASAP.

Until then the workaround would be -as you mentioned- to provide all git information manually. The following line allows me to build HAL from tarball (albeit very ugly): cmake -DHAL_VERSION_MAJOR=4 -DHAL_VERSION_MINOR=1 -DHAL_VERSION_PATCH=0 -DHAL_VERSION_TWEAK=0 -DHAL_VERSION_ADDITIONAL_COMMITS=160 -DHAL_VERSION_DIRTY=true -DHAL_VERSION_BROKEN=true -DCMAKE_BUILD_TYPE=Debug -DBUILD_ALL_PLUGINS=ON ..

xiota commented 1 year ago

Thank you for your response. I will wait for a tarball update. However, for the workaround, I do wonder why you chose to set the following to build from a tarball:

HAL_VERSION_ADDITIONAL_COMMITS=160  # instead of 0
HAL_VERSION_DIRTY=true              # instead of false
HAL_VERSION_BROKEN=true             # instead of false
joern274 commented 1 year ago

@xiota I was simply copying the values from my build directory (hal_version.h). Looks like I have done a lot of additional commits since 4.1.0 was released :smile: .

I cannot find any relevant line of code where the values mentioned above are used. The only requirement is that the type matches to avoid compile-time errors.

joern274 commented 1 year ago

The root cause of this problem was that parsing the HAL version number from file failed when no github information was available. It has been fixed by now.

We are going to release version 4.2.0 within a couple of days. Starting from that version it will be possible again to build HAL from tarball.

xiota commented 1 year ago

Thank you. Do you want me to test the change before the release? Otherwise, I'll wait for the release to test.

joern274 commented 1 year ago

@xiota Testers (especially high qualified testers like you) are always welcome but I am pretty confident that this particular issue is solved by now. To reproduce the "tarball" situation I copied the entire HAL directory to a different location and purged all .git related files. Doing so I could pinpoint the problem you reported to a cmake file parsing bug which was easy to fix. And again, you are very welcome to download the latest source and verify.

Since there were other changes in the code which are way more significant (like new base classes for plugin management) we decided to release 4.2.0 very soon instead of 4.1.1. Right now it is mostly documentation missing and then we are ready to go.

RenWal commented 1 year ago

@xiota Would you mind having a look at the tarball for release 4.2.0?

xiota commented 1 year ago

The following appears to succeed:

mkdir -p build && cd build
cmake -DCMAKE_INSTALL_PREFIX=/usr ..
make

However, "make install" fails with:

make: *** No rule to make target 'install'.  Stop.

I will try later to build/install with cmake directly.

xiota commented 1 year ago

Build from tarball with either make or cmake --build seems to work, but install with both make install and cmake --install seems to fail for different reasons. Will close this and open a new issue.