Skip to content

Conversation

@anibalsolon
Copy link

@anibalsolon anibalsolon commented Nov 20, 2022

This PR:

  • changes cmake setup to use conan as its library manager
  • downloads and points unit tests to the data dir
  • changes the logic to use N+1 offset
  • implements the temp dir logic from python
  • documents on how to run things
  • improves example
  • improves unit testing based on the python implementation
  • implements benchmark run/stats
  • add CI using Github Actions

@anibalsolon anibalsolon marked this pull request as draft November 20, 2022 22:29
@anibalsolon anibalsolon requested a review from frheault November 23, 2022 14:51
Copy link
Contributor

@frheault frheault left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the changes look great!

The next item on the list is about .
Maybe after that item, I can try to run the test on my new computer and see if it works?

@arokem
Copy link
Contributor

arokem commented Jan 9, 2023

On my mac, I am currently seeing:

(base) ➜  build git:(enh/devops) cmake .. && make
-- The C compiler identification is AppleClang 11.0.3.11030032
-- The CXX compiler identification is AppleClang 11.0.3.11030032
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /Library/Developer/CommandLineTools/usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /Library/Developer/CommandLineTools/usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Conan: Adjusting output directories
-- Conan: Using cmake global configuration
-- Conan: Adjusting default RPATHs Conan policies
-- Conan: Adjusting language standard
-- Current conanbuildinfo.cmake directory: /Users/arokem/source/trx-cpp/build
-- Found GTest: /Users/arokem/.conan/data/gtest/cci.20210126/_/_/package/aafb7f47234990db9077ed87e3f6c6c9c0e84a95/lib/libgtest.a  
-- Configuring done
-- Generating done
-- Build files have been written to: /Users/arokem/source/trx-cpp/build
[ 25%] Building CXX object CMakeFiles/trx.dir/src/trx.cpp.o
In file included from /Users/arokem/source/trx-cpp/src/trx.cpp:1:
In file included from /Users/arokem/source/trx-cpp/include/trx/trx.h:297:
/Users/arokem/source/trx-cpp/src/trx.tpp:1089:39: error: use of undeclared
      identifier 'canonicalize_file_name'
        std::string directory = (std::string)canonicalize_file_name(path...
                                             ^
1 error generated.
make[2]: *** [CMakeFiles/trx.dir/src/trx.cpp.o] Error 1
make[1]: *** [CMakeFiles/trx.dir/all] Error 2
make: *** [all] Error 2

@frheault
Copy link
Contributor

Great! It all worked!

Just so you know, here are small things I had to do:

After creating the profile, CONAN warn me of 'WARNING: GCC OLD ABI COMPATIBILITY' and told me I likely needed to run this command.
conan profile update settings.compiler.libcxx=libstdc++11 .conan
image

Then since I used CONAN before when I ran conan install --build=missing --settings=build_type=Debug --profile ./.conan .. I had to add the --update parameters.

Then CMAKE warned me that GTEST was missing,
image
so I googled it and I installed this:
sudo apt-get install libgtest-dev

And then the build and test worked!

PS: I see the file in ./examples/load_trx.cpp should work but and I see it in the CMAKE, but where is it being built?

@anibalsolon
Copy link
Author

@frheault good that it is working now- yes, this command is included in the README

The gtest should have been installed via conan, not sure why it didnt find

@frheault
Copy link
Contributor

frheault commented Feb 6, 2023

@anibalsolon Was I doing something wrong or was the file in ./examples/load_trx.cpp not build?

@frheault
Copy link
Contributor

@anibalsolon Is there something I can run or test before of meeting this afternoon?

@anibalsolon
Copy link
Author

@frheault not really- we may need to discuss some design decisions about the cpp implementation today

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.

4 participants